maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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