Hi, Alexander! The new patch is ok to push. Thanks! Some answers below. On Oct 09, Alexander Barkov wrote:
diff --git a/sql/item.cc b/sql/item.cc index fdfbba3..73a1acd 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -1407,22 +1407,82 @@ bool Item::get_date(MYSQL_TIME *ltime,ulonglong fuzzydate) return null_value|= !(fuzzydate & TIME_FUZZY_DATES); }
-bool Item::get_seconds(ulonglong *sec, ulong *sec_part) + +bool Item::get_seconds(Secondf_st *sec, const char *type, ulonglong maxsec) { - if (decimals == 0) - { // optimize for an important special case - longlong val= val_int(); - bool neg= val < 0 && !unsigned_flag; - *sec= neg ? -val : val; - *sec_part= 0; - return neg; + switch (cmp_type()) { + case STRING_RESULT: + return get_seconds_from_string(sec, type, maxsec); + case TIME_RESULT: + case DECIMAL_RESULT: + if (decimals == 0) // optimize for an important special case + return get_seconds_from_int(sec, type, maxsec); + return get_seconds_from_decimal(sec, type, maxsec); + case INT_RESULT: + return get_seconds_from_int(sec, type, maxsec); + case REAL_RESULT: + return get_seconds_from_real(sec, type, maxsec); + case ROW_RESULT: + case IMPOSSIBLE_RESULT: + break; }
I'm not sure that's the same as before. First, you completely ignore decimals when converting from double.
I think I am not. get_seconds_from_real() uses this method:
void Secondf_st::set(double value) { if ((m_neg= value < 0)) value= -value; if (value > LONGLONG_MAX) value= static_cast<double>(LONGLONG_MAX); m_second= static_cast<ulonglong>(floor(value)); m_usecond= static_cast<ulong>((value-floor(value))*TIME_SECOND_PART_FACTOR); }
I mean, the value of `decimals` property. You don't truncate m_usecond to have only `decimals` digits after the dot, it'll always have 6.
Second, old code was doing item->val_decimal(), while you're doing, for example, item->val_str() + str2my_decimal(). Which is not always the same.
Item::val_decimal_from_string() uses exactly the same approach. All Items with STRING_RESUL should go through this chain: val_str + str2my_decimal.
What about Item_hex_hybrid ? enum Item_result result_type () const { return STRING_RESULT; } String *val_str(String*) { return &str_value; } my_decimal *val_decimal(my_decimal *decimal_value) { ulonglong value= (ulonglong) Item_hex_hybrid::val_int(); int2my_decimal(E_DEC_FATAL_ERROR, value, TRUE, decimal_value); return decimal_value; }
- ltime->neg= sign; - if (sec > TIME_MAX_VALUE_SECONDS) + ltime->neg= sec.m_neg; + if (sec.m_second > TIME_MAX_VALUE_SECONDS) goto overflow;
kinda silly. get_seconds() checks for the overflow and you need to check it here again.
Notice, the maxsec value passed to gets_seconds() is not always the same with what the caller later checks again. Only Item_func_sec_to_time::get_date() passes maxsec=TIME_MAX_VALUE_SECONDS, while the other two calls pass LONGLONG_MAX.
This way I wanted to preserve the behavior in 5.5 a much as possible.
Yes, I understand why you introduced it and why the limit was re-checked. I didn't suggest a better approach, I didn't see it. It was just a comment that it looked rather strange to have a function that checks for the overflow, and in all places where it's called explicitly check for the overflow after the function call. Regards, Sergei Chief Architect MariaDB and security@mariadb.org