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 <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;
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 <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!