[Maria-developers] MDEV-4511 Assertion `scale <= precision' fails on GROUP BY TIMEDIFF with incorrect types
Hello Sergei, Please review a new version of the patch for MDEV-4511, it also fixes more crashing problems found, as well as non-crashing problems reported in MDEV-6302 I removed find_num_type() and fix_num_length_and_dec() which were hard to track, and for some Items these methods were erroneously called twice. fix_num_length_and_dec() now only exists as a non-virtual method in Item_sum and Item_udf_func. The patch is for 5.3, but it is still valid for 10.0, as we still have temporal functions with NOT_FIXED_DEC in 10.0. Thanks.
Hi, Alexander!
I removed find_num_type() and fix_num_length_and_dec() which were hard to track, and for some Items these methods were erroneously called twice.
fix_num_length_and_dec() now only exists as a non-virtual method in Item_sum and Item_udf_func.
Why did you keep them?
=== modified file 'sql/item_func.cc' --- sql/item_func.cc 2014-06-04 16:32:57 +0000 +++ sql/item_func.cc 2014-06-09 07:58:55 +0000 @@ -1618,8 +1606,20 @@ my_decimal *Item_func_div::decimal_op(my
void Item_func_div::result_precision() { + /* + We need to add args[1]->divisor_precision_increment(), + to properly handle the cases like this: + SELECT 5.05 / 0.014; -> 360.714286 + i.e. when the divisor has a zero integer part + and non-zero digits appear only after the decimal point. + Precision in this example is calculated as + args[0]->decimal_precision() + // 3 + args[1]->divisor_precision_increment() + // 3 + prec_increment // 4 + which gives 10 decimals digits. + */ uint precision=min(args[0]->decimal_precision() + - args[1]->decimals + prec_increment, + args[1]->divisor_precision_increment() + prec_increment,
I don't understand that, why divisor_precision_increment() and not decimals (or decimal_scale()) ?
DECIMAL_MAX_PRECISION);
/* Integer operations keep unsigned_flag if one of arguments is unsigned */
Regards, Sergei
Hello Sergei, On 07/23/2014 08:50 PM, Sergei Golubchik wrote:
Hi, Alexander!
I removed find_num_type() and fix_num_length_and_dec() which were hard to track, and for some Items these methods were erroneously called twice.
fix_num_length_and_dec() now only exists as a non-virtual method in Item_sum and Item_udf_func.
Why did you keep them?
Thanks for noticing this. It seems the only thing that Item_sum::fix_length_and_dec() and Item_udf_func::fix_length_and_dec() do is a call for fix_num_length_and_dec(). I will remove them.
=== modified file 'sql/item_func.cc' --- sql/item_func.cc 2014-06-04 16:32:57 +0000 +++ sql/item_func.cc 2014-06-09 07:58:55 +0000 @@ -1618,8 +1606,20 @@ my_decimal *Item_func_div::decimal_op(my
void Item_func_div::result_precision() { + /* + We need to add args[1]->divisor_precision_increment(), + to properly handle the cases like this: + SELECT 5.05 / 0.014; -> 360.714286 + i.e. when the divisor has a zero integer part + and non-zero digits appear only after the decimal point. + Precision in this example is calculated as + args[0]->decimal_precision() + // 3 + args[1]->divisor_precision_increment() + // 3 + prec_increment // 4 + which gives 10 decimals digits. + */ uint precision=min(args[0]->decimal_precision() + - args[1]->decimals + prec_increment, + args[1]->divisor_precision_increment() + prec_increment,
I don't understand that, why divisor_precision_increment() and not decimals (or decimal_scale()) ?
For two reasons: 1. The implementation is slightly more complex than just returning "decimals". See item.h: uint divisor_precision_increment() const { return decimals >= NOT_FIXED_DEC && is_temporal_type_with_time(field_type()) ? TIME_SECOND_PART_DIGITS : decimals; } and it's slightly different from decimal_scale(). 2. See the comment in item.h, near the method declaration. In the future we can make it virtual to get smaller data types: The items that never have zero integer part do not add digits into division results.
DECIMAL_MAX_PRECISION);
/* Integer operations keep unsigned_flag if one of arguments is unsigned */
Regards, Sergei
Hi, Alexander! On Jul 24, Alexander Barkov wrote:
uint divisor_precision_increment() const { return decimals >= NOT_FIXED_DEC && is_temporal_type_with_time(field_type()) ? TIME_SECOND_PART_DIGITS : decimals; }
Could you please rewrite divisor_precision_increment() as: uint divisor_precision_increment() const { return decimals < NOT_FIXED_DEC ? decimals : is_temporal_type_with_time(field_type()) ? TIME_SECOND_PART_DIGITS : decimals; } it's equivalent to your original expression, but makes it obvious the difference between divisor_precision_increment() and decimal_scale(). Thanks! Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik