maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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 <bar@xxxxxxxxxxx <mailto: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;

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 <bar@xxxxxxxxxxx
> <mailto: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!
>