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 <[email protected] <mailto:[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; 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<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] > <mailto:[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
