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