maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Jun 27, Alexander Barkov wrote:
> >> === modified file 'sql/field.cc'
> >> --- sql/field.cc	2013-06-06 19:32:29 +0000
> >> +++ sql/field.cc	2013-06-19 04:51:38 +0000
> >> @@ -87,6 +87,7 @@ const char field_separator=',';
> >>   #define FIELDTYPE_NUM (FIELDTYPE_TEAR_FROM + (255 - FIELDTYPE_TEAR_TO))
> >>   static inline int field_type2index (enum_field_types field_type)
> >>   {
> >> +  field_type= real_type_to_type(field_type);
> >
> > I don't like that MYSQL_TYPE_*2 types (which are, really, now only a
> > migration types, for mysql-5.6 data files) are leaking into the server.
> > I'd prefer if they'd stay in their corresponding Field objects
> > and the rest of the server wouldn't need to know they exist.
> 
> Can you clarify what exactly you suggest?

As much as possible (this is a disclaimer, as I am not sure to what
extend it is possible) to hide the differences.

Like, you have Field_timef, for example, that knows how MYSQL_TYPE_TIME2
is encoded in the row. But this field doesn't tell the rest of the
server about it, it returns MYSQL_TYPE_TIME, and in all respects behaves
just as any normal MYSQL_TYPE_TIME field. Only methods directly affected
by the storage format (::store, ::val_xxx, pack_length, whatever) are
different in this class.

And a little bit in the RBR code, that should know how to treat incoming
MYSQL_TYPE_TIME2 columns. That's all.

> >> @@ -4690,6 +4767,16 @@ String *Field_timestamp::val_str(String
> >>     *to++= (char) ('0'+(char) (temp));
> >>     *to= 0;
> >>     val_buffer->set_charset(&my_charset_numeric);
> >> +
> >> +  if (uint dec= decimals())
> >
> > eh? We never declare variables inside if(). A question of a consistent
> > coding style.
> 
> Will fix this.
> Btw. It looks convenient. Do you know why we don't use this?

Historically?

> >> @@ -5092,7 +5198,18 @@ int Field_temporal::store(double nr)
> >>   }
> >>
> >>
> >> -int Field_temporal::store(longlong nr, bool unsigned_val)
> >> +int Field_temporal::store_decimal(const my_decimal *d)
> >> +{
> >> +  ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED;
> >> +  double val;
> >> +  /* TODO: use decimal2string? */
> >> +  int err= warn_if_overflow(my_decimal2double(E_DEC_FATAL_ERROR &
> >> +                                            ~E_DEC_OVERFLOW, d, &val));
> >
> > decimal2string is slow, decimal2double is lossy.
> > Use my_decimal2seconds
> 
> decimal2double() should not be lossy for the TIME(0) and DATETIME(0) 
> types. DATETIME(0) needs 14 digits. The double type provides precision
> of 15 digits. Note, This is how it was before my patch.
> The zero precision data types TIME(0) and DATETIME(0) used
> my_decimal2double() through Field_str::store_decimal().
> I just did not change this behaviour, which is well tested
> through the years (which is good).

decimal2double() will be lossy doe DATETIME(6) I suppose.

> But I had to copy Field_str::store_decimal to 
> Field_temporal::store_decimal, which introduced
> a little bit of duplicate code (which is bad).
> 
> What is known to be faster?
> my_decimal2seconds() or my_decimal2double()?

my_decimal2seconds(). It's only integer arithmetics, not floating point.
Only for ranges, valid for temporal values, it doesn't try to handle
arbitrarily large DECIMALs (it'll return an overflow). It's basically an
optimized version, specially created for temporal values.

> >> === modified file 'sql/field.h'
> >> --- sql/field.h	2013-06-06 15:51:28 +0000
> >> +++ sql/field.h	2013-06-19 09:46:34 +0000
> >> @@ -1345,7 +1407,67 @@ class Field_null :public Field_str {
> >> +                    const ErrConv *str, int was_cut, timestamp_type ts_type);
> >> +  double pos_in_interval(Field *min, Field *max)
> >> +  {
> >> +    return pos_in_interval_val_str(min, max, 0);
> >
> > Really? This would first convert a temporal value to a string,
> > and then use a quite complicated procedure of placing a string
> > in the interval. Wouldn't it be much cheaper to use
> > pos_in_interval_val_real() ?
> 
> A native method should obviously be written eventially for
> the temporal data types.
> 
> Now I just left it how it worked before my patch:
> temporal data types used the method from their former
> parent Field_str.
> 
> I can change to pos_in_interval_val_real().
> But it does not have enough precision for all types, e.g. DATETIME(6).
> 
> What do you suggest?

I'd use pos_in_interval_val_real(). This method is used from the
optimizer, that is, quite often. It's not only for ANALYZE TABLE.

Precision loss isn't nice. But it'll only be an issue if the column
contains many (more than 10%) DATETIME values that differ only in
microseconds. In that case, precision loss might cause the statistics to
be skewed. Otherwise it doesn't matter - normally, even without
microseconds DATETIME values will be put in baskets correctly.

By the way, DECIMAL fields have the same problem.

Ideally, of course, we'd want to have pos_in_interval_val_decimal(),
it'd solve the problem both for DECIMAL and DATETIME.

> >> +  }
> >> +};
> >> +class Field_temporal_with_date: public Field_temporal {
> >> +protected:
> >> +  int store_TIME_with_warning(MYSQL_TIME *ltime, const ErrConv *str,
> >> +                              int was_cut, int have_smth_to_conv);
> >
> > Eh? So you'll have *three* different store_TIME_with_warning() methods?
> > I didn't like my version, because I had *two*,
> > there should've been only one.
> 
> I don't see any harm with that:
...
> Looks like a very natural change for me.

Ok, it'd difficult to see in the patch, so let's keep it your way.
I might look at it again, when you push.

> >> === modified file 'sql/opt_range.cc'
> >> --- sql/opt_range.cc	2013-06-06 19:32:29 +0000
> >> +++ sql/opt_range.cc	2013-06-18 10:35:35 +0000
> >> @@ -8014,10 +8014,10 @@ get_mm_leaf(RANGE_OPT_PARAM *param, COND
> >>
> >>     */
> >>     if (field->result_type() == STRING_RESULT &&
> >
> > this and below should probably use cmp_type, not result_type
> Probably. Why not to do it in a separate patch?

ok

Regards,
Sergei