Hi Alexander, Comments inline. Please provide feedback on why that Field_enum::can_optimize_range is needed. Ok to push otherwise. Feel free to address stylistic comments if you agree with them.
Vicențiu > commit 1d9f9b8d93fd0d1aad56b77694fb3c2b5a55514d > Author: Alexander Barkov <[email protected]> > 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; 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, just like the previous comment. Also, Is the static_cast necessary? > - 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<const Type_handler*>(&type_handler_var_string) : > + static_cast<const Type_handler*>(&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 <[email protected]> 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! >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
