Hello Sergei, On 10/26/2017 07:29 PM, Sergei Golubchik wrote:

Hi, Alexander!

On Oct 04, Alexander Barkov wrote:

...

Hello Sergei,

Please review a patch for MDEV-13997.

It's needed for: MDEV-13995 MAX(timestamp) returns a wrong result near DST change and for: MDEV-4912 Add a plugin to field types (column types)

Thanks!

See some questions below. I'm not yet familiar with new 10.3 type handler code, not sure I understood everything correctly.

...

commit 55eb354cbbeef818d739c69a700528decacd963d Author: Alexander Barkov Date: Wed Oct 4 15:12:36 2017 +0400

diff --git a/sql/item.cc b/sql/item.cc index f609bfd..992215f 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -109,6 +109,19 @@ void Item::push_note_converted_to_positive_complement(THD *thd) }

+longlong Item::val_datetime_packed_result() +{ + MYSQL_TIME ltime, tmp; + if (get_date_result(<ime, TIME_FUZZY_DATES | TIME_INVALID_DATES)) + return 0; + if (ltime.time_type != MYSQL_TIMESTAMP_TIME) + return pack_time(<ime); + if ((null_value= time_to_datetime_with_warn(current_thd, <ime, &tmp, 0))) + return 0; + return pack_time(&tmp);

Hmm, it's used instead of the old Item_cache_temporal::cache_value() but it's significantly more complex. What's the change in behavior?

In the old code it was get_datetime_value() who stored the result into Item_cache_temporal. get_datetime_value() made sure to do TIME-to-DATETIME conversion when needed. It involved the following methods: longlong val_temporal_packed(enum_field_types f_type) { return f_type == MYSQL_TYPE_TIME ? val_time_packed() : val_datetime_packed(); } virtual longlong val_datetime_packed() { MYSQL_TIME ltime; ulonglong fuzzydate= TIME_FUZZY_DATES | TIME_INVALID_DATES; return get_date_with_conversion(<ime, fuzzydate) ? 0 : pack_time(<ime); } So in the old code, it was not important for Item_cache_temporal::cache_value() to care about TIME-to-DATETIME conversion, because get_datetime_value() took care about it. In the new code it is important to do this conversion in cache_value(). So Item::val_datetime_packed_result() now implements the same logic: if get_date_result() returned TIME, it gets converted to DATETIME before calling pack_time(). Btw, in the next patch I will fix Item_func_in to create a cache for the predicant expression at fix time rather than at execution time. So get_datetime_value() will be gone soon.

...

+} + + /** Get date/time/datetime. Optionally extend TIME result to DATETIME. diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc index 9954c66..90b150a 100644 --- a/sql/item_cmpfunc.cc +++ b/sql/item_cmpfunc.cc @@ -2112,38 +2102,40 @@ bool Item_func_between::fix_length_and_dec_numeric(THD *thd) }

-longlong Item_func_between::val_int_cmp_temporal() +bool Item_func_between::fix_length_and_dec_temporal(THD *thd) { - THD *thd= current_thd; - longlong value, a, b; - Item *cache, **ptr; - bool value_is_null, a_is_null, b_is_null; + if (!thd->lex->is_ps_or_view_context_analysis()) + { + for (uint i= 0; i < 3; i ++) + { + if (args[i]->const_item() && + args[i]->type_handler_for_comparison() != m_comparator.type_handler())

What does this condition mean?

If the data type of the comparison operation is different from the data type of the constant, we convert the constant. For example, if comparison is done as DATETIME and the constant is TIME, then the constant is converted to DATETIME. This is to safe on conversion at Item_func_between::val_int(). Note, in the old code get_datetime_value() took care of this conversion.

...

+ { + Item_cache *cache= m_comparator.type_handler()->Item_get_cache(thd, args[i]); + if (!cache || cache->setup(thd, args[i])) + return true; + thd->change_item_tree(&args[i], cache); + } + } + } + return false; +}

Regards, Sergei Chief Architect MariaDB and security@mariadb.org