MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:July 16 2009 7:53pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (davi:3013) Bug#45261
View as plain text  
Hi Sergei,

On 7/15/09 11:09 AM, Sergei Golubchik wrote:
> Hi, Davi!
>
> I have few questions, see below:
>
> On Jul 08, Davi Arnaut wrote:
>
>> === modified file 'mysql-test/r/type_newdecimal.result'
>> --- a/mysql-test/r/type_newdecimal.result	2009-07-03 10:36:04 +0000
>> +++ b/mysql-test/r/type_newdecimal.result	2009-07-08 13:18:39 +0000
>> @@ -1495,9 +1495,9 @@ CREATE TABLE t1 (a int DEFAULT NULL, b i
>>   INSERT INTO t1 VALUES (3,30), (1,10), (2,10);
>>   SELECT a+CAST(1 AS decimal(65,30)) AS aa, SUM(b) FROM t1 GROUP BY aa;
>>   aa	SUM(b)
>> -2.000000000000000000000000000000	10
>> -3.000000000000000000000000000000	10
>> -4.000000000000000000000000000000	30
>> +2.00000000000000000000000000000	10
>> +3.00000000000000000000000000000	10
>> +4.00000000000000000000000000000	30
>
> why ?

Because of the precision increase for additive operations and that the 
results are materialized (temporary table).

Just for the sake of argument, a clearer test case:

CREATE TABLE t1 AS SELECT 9 + CAST(1 AS decimal(65,30)) AS a;
DESC t1;

Field	Type	
a	decimal(65,29)

What happens is that we have a additive operation between a number that 
has a precision of 1 and one of precision 65. The integer parts are 1 
and 35 respectively. When adding the two, the precision is increased to 
66 as the integer part can increased by 1 in a additive operation (see 
Item_func_additive_op::result_precision). Hence we end up with a 
decimal(66,30) item.

Once the item is to be written to a table, the truncation procedure 
kicks in. Since the integer part fits, the decimals are truncated.

>>   SELECT a+CAST(1 AS decimal(65,31)) AS aa, SUM(b) FROM t1 GROUP BY aa;
>>   ERROR 42000: Too big scale 31 specified for column '1'. Maximum is 30.
>>   DROP TABLE t1;
>> @@ -1595,7 +1595,7 @@ Warnings:
>>   Note	1265	Data truncated for column 'my_col' at row 1
>>   DESCRIBE t1;
>>   Field	Type	Null	Key	Default	Extra
>> -my_col	decimal(65,30)	NO		0.000000000000000000000000000000	
>> +my_col	decimal(32,30)	NO		0.000000000000000000000000000000	
>
> why ?
> (I cannot check the query that caused that as these lines
> don't exist in my 5.4 or 6.0 trees)

Sorry, the test case added for Bug#45262 is not yet merged upstream:

+CREATE TABLE t1 SELECT 1 + 
.123456789123456789123456789123456789123456789123456789123456789123456789123456789 
AS my_col;
+Warnings:
+Note	1265	Data truncated for column 'my_col' at row 1
+DESC t1;
+Field	Type	Null	Key	Default	Extra
+my_col	decimal(32,30)	NO		0.000000000000000000000000000000	

The integer part fits and the decimal part is truncated. Reasoning above 
applies because of additive operation.

>>   SELECT my_col FROM t1;
>>   my_col
>>   1.123456789123456789123456789123
>> === modified file 'sql/item.cc'
>> --- a/sql/item.cc	2009-07-03 10:43:54 +0000
>> +++ b/sql/item.cc	2009-07-08 13:18:39 +0000
>> @@ -437,15 +437,13 @@ Item::Item(THD *thd, Item *item):
>>
>>   uint Item::decimal_precision() const
>>   {
>> +  uint precision= max_length;
>>     Item_result restype= result_type();
>>
>>     if ((restype == DECIMAL_RESULT) || (restype == INT_RESULT))
>> -  {
>> -    uint prec=
>> -      my_decimal_length_to_precision(max_length, decimals, unsigned_flag);
>> -    return min(prec, DECIMAL_MAX_PRECISION);
>> -  }
>> -  return min(max_length, DECIMAL_MAX_PRECISION);
>> +    precision= my_decimal_length_to_precision(max_length, decimals,
> unsigned_flag);
>> +
>> +  return precision;
>
> why capping the precision here was incorrect ?

Because capping the precision makes it impossible to calculate exactly 
what was the integer part. See Item::decimal_int_part.

> I mean, were there user-visible effect, like incorrect results ?

Yes:

+CREATE TABLE t1 (a DECIMAL(30,30));
+INSERT INTO t1 VALUES (0.1),(0.2),(0.3);
+CREATE TABLE t2 SELECT MIN(a + 0.0000000000000000000000000000001) FROM t1;
+SHOW CREATE TABLE t2;
+Table	Create Table
+t2	CREATE TABLE `t2` (
+  `MIN(a + 0.0000000000000000000000000000001)` decimal(33,31) <---
+) ENGINE=MyISAM DEFAULT CHARSET=latin1

But I missed the fix for the above (and probably other) cases:

=== modified file 'sql/item_sum.cc'
--- sql/item_sum.cc	2009-07-03 10:36:04 +0000
+++ sql/item_sum.cc	2009-07-16 19:45:24 +0000
@@ -517,8 +517,7 @@ Field *Item_sum::create_tmp_field(bool g
                                 name, table->s, collation.collation);
      break;
    case DECIMAL_RESULT:
-    field= new Field_new_decimal(max_length, maybe_null, name,
-                                 decimals, unsigned_flag);
+    field= make_new_decimal_field();
      break;
    case ROW_RESULT:
    default:

>>   }
>>
>>
>> === modified file 'sql/item_func.cc'
>> --- a/sql/item_func.cc	2009-07-03 10:36:04 +0000
>> +++ b/sql/item_func.cc	2009-07-08 13:18:39 +0000
>> @@ -452,45 +452,8 @@ Field *Item_func::tmp_table_field(TABLE
>>       return make_string_field(table);
>>       break;
>>     case DECIMAL_RESULT:
>> -  {
>> -    uint8 dec= decimals;
>> -    uint8 intg= decimal_precision() - dec;
>> -    uint32 len= max_length;
>> -
>> -    /*
>> -      Trying to put too many digits overall in a DECIMAL(prec,dec)
>> -      will always throw a warning. We must limit dec to
>> -      DECIMAL_MAX_SCALE however to prevent an assert() later.
>> -    */
>> -
>> -    if (dec>  0)
>> -    {
>> -      int overflow;
>> -
>> -      dec= min(dec, DECIMAL_MAX_SCALE);
>> -
>> -      /*
>> -        If the value still overflows the field with the corrected dec,
>> -        we'll throw out decimals rather than integers. This is still
>> -        bad and of course throws a truncation warning.
>> -      */
>> -
>> -      const int required_length=
>> -        my_decimal_precision_to_length(intg + dec, dec,
>> -                                                     unsigned_flag);
>> -
>> -      overflow= required_length - len;
>> -
>> -      if (overflow>  0)
>> -        dec= max(0, dec - overflow);            // too long, discard fract
>> -      else
>> -        /* Corrected value fits. */
>> -        len= required_length;
>> -    }
>> -
>> -    field= new Field_new_decimal(len, maybe_null, name, dec, unsigned_flag);
>> +    field= make_new_decimal_field();
>>       break;
>
> This wasn't part of your first patch. I don't see where in the test case
> you cover this change.

Covered by tests added in patch for Bug#45262

>> -  }
>>     case ROW_RESULT:
>>     default:
>>       // This case should never be chosen
>> === modified file 'sql/my_decimal.h'
>> --- a/sql/my_decimal.h	2009-07-03 10:36:04 +0000
>> +++ b/sql/my_decimal.h	2009-07-08 13:18:39 +0000
>> @@ -51,7 +51,6 @@ C_MODE_END
>>   */
>>   #define DECIMAL_MAX_PRECISION (DECIMAL_MAX_POSSIBLE_PRECISION - 8*2)
>>   #define DECIMAL_MAX_SCALE 30
>> -#define DECIMAL_NOT_SPECIFIED 31
>
> Oh, so it was unused ?
>

No, it was used in some strange way in my_decimal_int_part. Since the 
function was removed, it became unused.

Regards,

-- Davi Arnaut
Thread
bzr commit into mysql-5.1-bugteam branch (davi:3013) Bug#45261Davi Arnaut8 Jul
Re: bzr commit into mysql-5.1-bugteam branch (davi:3013) Bug#45261Davi Arnaut16 Jul
  • Re: bzr commit into mysql-5.1-bugteam branch (davi:3013) Bug#45261Sergei Golubchik18 Jul
Re: bzr commit into mysql-5.1-bugteam branch (davi:3013) Bug#45261Davi Arnaut17 Jul