Hello Vicentiu, Thanks for your review! See comments inline: On 04/24/2017 07:46 PM, Vicențiu Ciorbaru wrote:

Hi Alexander,

Comments inline. Please provide feedback on why that Field_enum::can_optimize_range is needed.

The core logic is the same. But the debugging logic is different: the inherited method has different DBUG_ASSERTs.

Ok to push otherwise. Feel free to address stylistic comments if you agree with them.

Vicențiu

...

commit 1d9f9b8d93fd0d1aad56b77694fb3c2b5a55514d Author: Alexander Barkov mailto:bar@mariadb.org> Date: Mon Apr 3 13:38:20 2017 +0400

diff --git a/sql/field.cc b/sql/field.cc index 362c49b..44b1acd 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -9049,12 +9054,17 @@ uint Field_num::is_equal(Create_field *new_field) }

Why is this needed? Field::can_optimize_range defines this exact logic and nothing in the inheritance tree (Field_str) overrides it.

...

+bool Field_enum::can_optimize_range(const Item_bool_func *cond, + const Item *item, + bool is_eq_func) const +{ + return item->cmp_type() != TIME_RESULT; +} + + bool Field_enum::can_optimize_keypart_ref(const Item_bool_func *cond, const Item *item) const { - DBUG_ASSERT(cmp_type() == INT_RESULT); - DBUG_ASSERT(result_type() == STRING_RESULT); - switch (item->cmp_type()) { case TIME_RESULT: diff --git a/sql/field.h b/sql/field.h index 2ed84b8..26593e3 100644 --- a/sql/field.h +++ b/sql/field.h @@ -2531,14 +2534,18 @@ class Field_year :public Field_tiny { :Field_tiny(ptr_arg, len_arg, null_ptr_arg, null_bit_arg, unireg_check_arg, field_name_arg, 1, 1) {} - enum_field_types type() const { return MYSQL_TYPE_YEAR;} + const Type_handler *type_handler() const { return &type_handler_year; } Copy_func *get_copy_func(const Field *from) const { if (eq_def(from)) return get_identical_copy_func(); switch (from->cmp_type()) { case STRING_RESULT: Can you write it as: if (handler == &type_handler_enum || handler == &type_handler_set) return do_field_int; return do_field_string;

Done.

I don't think that the ternary ? operator here adds much value and they way things are aligned makes it harder to read. (personal preference here, your call in the end)

...

- return do_field_string; + { + const Type_handler *handler= from->type_handler(); + return handler == &type_handler_enum || + handler == &type_handler_set ? do_field_int : do_field_string; + } case TIME_RESULT: return do_field_temporal; case DECIMAL_RESULT: @@ -3013,13 +3027,11 @@ class Field_string :public Field_longstr { NONE, field_name_arg, cs), can_alter_field_type(1) {};

I would do an if statement here,

done

just like the previous comment. Also, Is the static_cast necessary?

With the ternary operator ? they were necessary. Otherwise, the compiler returned an error, because the two options are of slightly different data types. But with "if" approach casts are not needed. I removed them.

...

- enum_field_types type() const + const Type_handler *type_handler() const { - return ((can_alter_field_type && orig_table && - orig_table->s->db_create_options & HA_OPTION_PACK_RECORD && - field_length >= 4) && - orig_table->s->frm_version < FRM_VER_TRUE_VARCHAR ? - MYSQL_TYPE_VAR_STRING : MYSQL_TYPE_STRING); + return is_var_string() ? + static_cast(&type_handler_var_string) : + static_cast(&type_handler_string); } enum ha_base_keytype key_type() const { return binary() ? HA_KEYTYPE_BINARY : HA_KEYTYPE_TEXT; } @@ -3541,6 +3545,9 @@ class Field_enum :public Field_str { */ return false; } Again, this should not be necessary. + bool can_optimize_range(const Item_bool_func *cond, + const Item *item, + bool is_eq_func) const; private: int do_save_field_metadata(uchar *first_byte); uint is_equal(Create_field *new_field); diff --git a/sql/item.cc b/sql/item.cc index 308ed1d..65f1e50 100644 --- a/sql/item.cc +++ b/sql/item.cc

On Mon, 3 Apr 2017 at 02:42 Alexander Barkov mailto:bar@mariadb.org> wrote:

Hello Vicențiu,

Please review a patch for:

MDEV-12426 Add Field::type_handler()

also fixing:

MDEV-12432 Range optimizer for ENUM and SET does not return "Impossible WHERE" in some case

Thank you!