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 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