maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hello Alexey,

On 03/10/2017 10:56 AM, Alexey Botchkov wrote:
> Ok, i see.
> 
> Last question then.
> -           item->cast_to_int_type() == cast_to_int_type() &&
> +           item->cast_to_int_type_handler() ==
> cast_to_int_type_handler() &&
>  Are you sure we should compare typehandlers here, not their ->cmp_type()?

Item_hex_constant can be:

1. Either Item_hex_hybrid, which can act as a string or as a number,
   depending on context:

SELECT 0x30, 0x30+1;
+------+--------+
| 0x30 | 0x30+1 |
+------+--------+
| 0    |     49 |
+------+--------+

(the first 0x30 is a string '0',
 the second 0x30 is an integer 0x30, which is decimal 48)

2. Or Item_hex_string, which is always a string:
SELECT X'30', X'30'+1;
+-------+---------+
| X'30' | X'30'+1 |
+-------+---------+
| 0     |       1 |
+-------+---------+
(both X'30's are strings '0')


The method Item_hex_constant::eq() needs to make sure that
X'30' and 0x30 are considered as different items, and thus
don't replace each other, e.g. in equal field propagation.

Item_hex_hybrid::cast_to_int_type_handler() returns &type_handler_longlong.

Item_hex_string::cast_to_int_type_handler() returns
&type_handler_varchar.


Comparing type handlers should be enough here.

Comparing cmp_type() would be also correct,
but would involve one extra virtual call, which is nice to avoid.


> 
> Also i don't like that we have the 'val_int_signed_typecast' as a
> virtual functions. Code would
> be much nicer without that and all it was done just for Item_dyncol_get.
> Not that simple to fix it though and I agree that it shouldn't be
> touched as a part of this patch.
> Probably sometime later...

Maybe. Perhaps some related code will also migrate to Type_handler.
Probably this would do:

virtual longlong Type_handler::val_int_signed_typecast(Item *item);

But I'm not 100% sure. Let's return to this later.


Thanks!

> 
> Best regards.
> HF
> 
> 
> 
> On Fri, Mar 10, 2017 at 1:10 AM, Alexander Barkov <bar@xxxxxxxxxxx
> <mailto:bar@xxxxxxxxxxx>> wrote:
> 
>     Hello Alexey,
> 
> 
>     On 03/09/2017 11:38 PM, Alexey Botchkov wrote:
>     > Hi, Alexander.
>     >
>     > Some questions about the patch.
>     >
>     > I don't get why we have two sets of functions
>     > Type_handler_**::+bool
>     > Type_handler_int_result::Item_func_abs_fix_length_and_dec
>     > and
>     > Type_handler_**::+bool
>     > Type_handler_int_result::Item_func_neg_fix_length_and_dec
>     >
>     > As far as i can see these are equal and we just can have one
>     > Type_handler_**::+bool
>     > Type_handler_int_result::Item_func_abs_neg_fix_length_and_dec
>     > Did I miss something?
> 
> 
>     They might look similar:
> 
> 
>     bool Type_handler_int_result::
>            Item_func_abs_fix_length_and_dec(Item_func_abs *item) const
>     {
>       item->fix_length_and_dec_int();
>       return false;
>     }
> 
>     bool Type_handler_int_result::
>            Item_func_neg_fix_length_and_dec(Item_func_neg *item) const
>     {
>       item->fix_length_and_dec_int();
>       return false;
>     }
> 
> 
>     But they have arguments of different pointer types!
> 
> 
>     The former calls Item_func_abs::fix_length_and_dec_int(), and
>     the latter calls Item_func_neg::fix_length_and_dec_int(),
> 
>     and those are different.
> 
>     >
>     > Secondly, since Item_func::fix_length_and_dec() was historically 'void'
>     > and i don't see you're
>     > changing it, why do all these typehandler's 'xxx_fix_length_and_dec'
>     > return something?
>     > Nobody checks their return anyway.
> 
> 
>     Currently fix_fields() does this:
> 
>       fix_length_and_dec();
>       if (thd->is_error()) // An error inside fix_length_and_dec occurred
>         return TRUE;
> 
>     I'm thinking of eventually changing this to:
> 
>       if (fix_length_and_dec())
>         return true;
> 
> 
>     So I decided to make all Type_handler::xxxx_fix_length_and_dec()
>     return bool right now, so we have to do less changes in the future.
> 
> 
>     Thanks!
> 
>     >
>     > Best regards.
>     > HF
>     >
>     >
>     > On Tue, Mar 7, 2017 at 6:15 PM, Alexander Barkov <bar@xxxxxxxxxxx <mailto:bar@xxxxxxxxxxx>
>     > <mailto:bar@xxxxxxxxxxx <mailto:bar@xxxxxxxxxxx>>> wrote:
>     >
>     >     Hello Alexey,
>     >
>     >     Please review a patch for MDEV-12199.
>     >
>     >     Thanks!
>     >
>     >
> 
>