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