Re: [Maria-developers] Please review MDEV-12199 Split Item_func_{abs|neg|int_val}::fix_length_and_dec() into methods in Type_handler

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 mailto:bar@mariadb.org> 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 mailto:bar@mariadb.org > mailto:bar@mariadb.org>> wrote: > > Hello Alexey, > > Please review a patch for MDEV-12199. > > Thanks! > >