Hi Sergei,
On 03/16/2016 09:51 PM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> On Feb 08, Alexander Barkov wrote:
>> Hello Sergei,
>>
>> Please review a patch for MDEV-9369.
>>
>> Briefly, it fixes cmp_item::cmp() to return three possible values:
>> FALSE, TRUE, UNKNOWN. These values are then recursively pulled to
>> the very top level, to the owner Item_xxx.
>>
>> The owner Item_func_in (as well as Item_func_case and Item_func_equal)
>> then further "translate" these tree-values logic into the traditional
>> val_int()+null_value combination using some additional information.
>>
>> For example, Item_func_in::val_int() uses "negated" as an additional
>> information.
>>
>> The patch looks Ok for me.
>
> Looks ok to me too. A couple of stylistic comments, see below.
> ok to push afterwards.
>
>> diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
>> index d5f5087..39003de 100644
>> --- a/sql/item_cmpfunc.cc
>> +++ b/sql/item_cmpfunc.cc
>> @@ -3837,15 +3843,15 @@ void cmp_item_decimal::store_value(Item *item)
>>/* val may be zero if item is nnull */
>>if (val && val != &value)
>> my_decimal2decimal(val, &value);
>> + set_null_value(item->null_value);
>> }
>>
>>
>> int cmp_item_decimal::cmp(Item *arg)
>> {
>>my_decimal tmp_buf, *tmp= arg->val_decimal(&tmp_buf);
>> - if (arg->null_value)
>> -return 1;
>> - return my_decimal_cmp(&value, tmp);
>> + return (m_null_value || arg->null_value) ?
>
> if you access m_null_value directly, you don't need a setter for it.
> it'll be less confusing if you replace set_null_value(X) with m_null_value=X.
>
> alternatively, you can add a getter is_null_value(). But I personally
> wouldn't do it, cmp_xxx is a small set of simple classes, no need to get
> too enterprisey there.
Removed set_null_value(), changed to use m_null_value directly.
>
>> +UNKNOWN : (my_decimal_cmp(&value, tmp) != 0);
>> }
>>
>>
>> @@ -3892,10 +3900,10 @@ cmp_item *cmp_item_datetime::make_same()
>> }
>>
>>
>> -bool Item_func_in::nulls_in_row()
>> +bool Item_func_in::list_contains_null()
>
> I found the old name clear enough. But ok, whatever...
>
>> {
>>Item **arg,**arg_end;
>> - for (arg= args+1, arg_end= args+arg_count; arg != arg_end ; arg++)
>> + for (arg= args + 1, arg_end= args+arg_count; arg != arg_end ; arg++)
>>{
>> if ((*arg)->null_inside())
>>return 1;
>> @@ -4010,6 +4018,32 @@ void Item_func_in::fix_length_and_dec()
>> }
>>}
>>
>> + /*
>> +First conditions for bisection to be possible:
>> + 1. All types are similar, and
>> + 2. All expressions in are const
>> + */
>> + bool bisection_possible=
>> +type_cnt == 1 && // 1
>> +const_itm; // 2
>> + if (bisection_possible)
>> + {
>> +/*
>> + In the presence of NULLs, the correct result of evaluating this item
>> + must be UNKNOWN or FALSE. To achieve that:
>> + - If type is scalar, we can use bisection and the "have_null" boolean.
>> + - If type is ROW, we will need to scan all of when
>> +searching, so bisection is impossible. Unless:
>> +3. UNKNOWN and FALSE are equivalent results
>> +4. Neither left expression nor contain any NULL
>> value
>> + */
>> +
>> +if (cmp_type == ROW_RESULT &&
>> +!((is_top_level_item() && !negated) || // 3
>> + (!list_contains_null() && !args[0]->maybe_null))) // 4
>> + bisection_possible= false;
>
> I think the condition will be easier to understand if you remove the
> negation of that complex condition in parentheses:
>
> if (cmp_type == ROW_RESULT &&
> ((!is_top_level_item() || negated) && // 3
> (list_contains_null() || args[0]->maybe_null))) // 4
>
> I find this ^^^ much easier to understand.
Agreed. Fixed to use your version.
Thanks for review! I pushed the patch.
>
>> + }
>> +
>>if (type_cnt == 1)
>>{
>> if (cmp_type == STRING_RESULT &&
>
> Regards,
> Sergei
> Chief Architect MariaDB
> and secur...@mariadb.org
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
