maria-developers team mailing list archive

[Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> 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@xxxxxxxxxxx> 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!
>