maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

>> Added typedef decimal_digits_t (uint16) for number of decimals
>> For fields and Item's uint8 should be good enough. After
>> discussions with Alexander Barkov we choose uint16 (for now)
>> as some format functions may accept +256 digits.
>>
>> The reason for this patch was to make the storage of decimal
>> digits simlar. Before this patch decimals was stored/used as
>> uint8, int and uint.
>>
>> Changed most decimal variables and functions to use the new
>> typedef.
>>
>> diff --git a/include/decimal.h b/include/decimal.h
>> index cab18f99348..f713409ede1 100644
>> --- a/include/decimal.h
>> +++ b/include/decimal.h
>> @@ -53,10 +53,11 @@ int decimal2double(const decimal_t *from, double *to);
>>  int double2decimal(double from, decimal_t *to);
>>  int decimal_actual_fraction(const decimal_t *from);
>>  int decimal2bin(const decimal_t *from, uchar *to, int precision, int scale);
>> -int bin2decimal(const uchar *from, decimal_t *to, int precision, int scale);
>> +int bin2decimal(const uchar *from, decimal_t *to, int precision,
>> +                decimal_digits_t scale);
>>
>> -int decimal_size(int precision, int scale);
>> -int decimal_bin_size(int precision, int scale);
>> +int decimal_size(int precision, decimal_digits_t scale);
>> +int decimal_bin_size(int precision, decimal_digits_t scale);
>
>this is *very* confusing.
>
>"decimal" in the context of an Item mean "number of digits after a dot"
>"decimal" in decimal.h means the whole number, and after a dot is "scale"

The intention with the patch was to have (over time) one type for all
lengths associated with length-of-numbers. This include precision,
scale, digits before '.', digits after '.' etc.

I started with 'digits after dot', and then fixed other related things.

I have now, by your request, created a full patch that fixes all of the
above.

<cut>

>> -static uint get_decimal_precision(uint len, uint8 dec, bool unsigned_val)
>> +static decimal_digits_t get_decimal_precision(uint len, decimal_digits_t dec,
>> +                                              bool unsigned_val)
>>  {
>>    uint precision= my_decimal_length_to_precision(len, dec, unsigned_val);
>> -  return MY_MIN(precision, DECIMAL_MAX_PRECISION);
>> +  return (decimal_digits_t) MY_MIN(precision, DECIMAL_MAX_PRECISION);
>
>No, this is wrong (or, rather, inconsistent with your other changes).
>"precision" for DECIMAL is the total number of digits, not a number of
>digits after the dot. Judging by your edits in decimal.h you don't want
>that to be decimal_digits_t.

It's not inconsistent or a contradiction, it is an extension ;)
I agree that I should have mentioned in the commit comment that the new
type is also used for precision in some places.
Anyway, the new patch should fix our concerns.

<cut>

>>    {
>>      return type_handler()->Item_decimal_scale(this);
>>    }
>> @@ -4853,7 +4853,7 @@ class Item_temporal_literal :public Item_literal
>>      Item_literal(thd)
>>    {
>>      collation= DTCollation_numeric();
>> -    decimals= dec_arg;
>> +    decimals= (decimal_digits_t) dec_arg;
>
>why do you cast? I'd expect dec_arg to be decimal_digits_t already

  Item_temporal_literal(THD *thd, uint dec_arg):
    Item_literal(thd)
  {
    collation= DTCollation_numeric();
    decimals= (decimal_digits_t) dec_arg;
  }

It was not. I will fix that.
I only add cast when the compiler requires or asks for it..

>> diff --git a/sql/item_func.cc b/sql/item_func.cc
>> index a0ef4020aae..db51639a5af 100644
>> --- a/sql/item_func.cc
>> +++ b/sql/item_func.cc
>> @@ -2710,7 +2710,7 @@ my_decimal *Item_func_round::decimal_op(my_decimal *decimal_value)
>>      dec= INT_MIN;
>>
>>    if (!(null_value= (value.is_null() || args[1]->null_value ||
>> -                     value.round_to(decimal_value, (uint) dec,
>> +                     value.round_to(decimal_value, (int) dec,
>>                                      truncate ? TRUNCATE : HALF_UP) > 1)))
>
>I don't think you need to cast decimal_digits_t (aka uint16) to int
>explicitly. A compiler can handle it, it's not lossy.

Here dec is calculated is a longlong, so it needs a cast.
(We got this from a val_int() call).

Note that in this particular place, dec can be negative and that is ok
for value.round().
The orginal code was wrong. It should never have been casted to an int.

<cut>

>> --- a/sql/item_func.h
>> +++ b/sql/item_func.h
>> @@ -1383,10 +1383,10 @@ class Item_decimal_typecast :public Item_func
>>  {
>>    my_decimal decimal_value;
>>  public:
>> -  Item_decimal_typecast(THD *thd, Item *a, uint len, uint dec)
>> +  Item_decimal_typecast(THD *thd, Item *a, uint len, decimal_digits_t dec)
>>     :Item_func(thd, a)
>>    {
>> -    decimals= (uint8) dec;
>> +    decimals= (decimal_digits_t)  dec;
>
>not needed, dec is decimal_digits_t already

thanks.  I probably just replaced one cast, for which I got a warning,
with another.

>
>>      collation= DTCollation_numeric();
>>      fix_char_length(my_decimal_precision_to_length_no_truncation(len, dec,
>>                                                                   unsigned_flag));
>> diff --git a/sql/sql_type.cc b/sql/sql_type.cc
>> index 5a31b39c7b6..1c5e03f34bd 100644
>> --- a/sql/sql_type.cc
>> +++ b/sql/sql_type.cc
>> @@ -1218,9 +1218,10 @@ uint32 Type_numeric_attributes::find_max_octet_length(Item **item, uint nitems)
>>  }
>>
>>
>> -int Type_numeric_attributes::find_max_decimal_int_part(Item **item, uint nitems)
>> +decimal_digits_t Type_numeric_attributes::
>> +find_max_decimal_int_part(Item **item, uint nitems)
>>  {
>> -  int max_int_part= 0;
>> +  decimal_digits_t max_int_part= 0;
>
>again, "int parts" = precision-scale.
>it's not clear from your patch whether it should be decimal_digits_t

I assume it will be more clear in the new patch.

Here is the new patch, on top of the old one.

Regards,
Monty

diff --git a/strings/decimal.c b/strings/decimal.c
index 16bc887814a..1f9a28c1ad5 100644
--- a/strings/decimal.c
+++ b/strings/decimal.c
@@ -2619,7 +2619,8 @@ void test_d2f(const char *s, int ex)
 
 void test_d2b2d(const char *str, int p, int s, const char *orig, int ex)
 {
-  char s1[100], buf[100], *end;
+  char s1[100], *end;
+  uchar buf[100];
   int res, i, size=decimal_bin_size(p, s);
 
   sprintf(s1, "'%s'", str);
@@ -2937,27 +2938,27 @@ int main()
   test_f2d(1234500009876.5, 0);
 
   printf("==== ulonglong2decimal ====\n");
-  test_ull2d(ULL(12345), "12345", 0);
-  test_ull2d(ULL(0), "0", 0);
-  test_ull2d(ULL(18446744073709551615), "18446744073709551615", 0);
+  test_ull2d(12345ULL, "12345", 0);
+  test_ull2d(0ULL, "0", 0);
+  test_ull2d(18446744073709551615ULL, "18446744073709551615", 0);
 
   printf("==== decimal2ulonglong ====\n");
   test_d2ull("12345", "12345", 0);
   test_d2ull("0", "0", 0);
   test_d2ull("18446744073709551615", "18446744073709551615", 0);
-  test_d2ull("18446744073709551616", "18446744073", 2);
+  test_d2ull("18446744073709551616", "18446744073709551615", 2);
   test_d2ull("-1", "0", 2);
   test_d2ull("1.23", "1", 1);
-  test_d2ull("9999999999999999999999999.000", "9999999999999999", 2);
+  test_d2ull("9999999999999999999999999.000", "18446744073709551615", 2);
 
   printf("==== longlong2decimal ====\n");
-  test_ll2d(LL(-12345), "-12345", 0);
-  test_ll2d(LL(-1), "-1", 0);
-  test_ll2d(LL(-9223372036854775807), "-9223372036854775807", 0);
-  test_ll2d(ULL(9223372036854775808), "-9223372036854775808", 0);
+  test_ll2d(-12345LL, "-12345", 0);
+  test_ll2d(-1LL, "-1", 0);
+  test_ll2d(-9223372036854775807LL, "-9223372036854775807", 0);
+  test_ll2d(9223372036854775808ULL, "-9223372036854775808", 0);
 
   printf("==== decimal2longlong ====\n");
-  test_d2ll("18446744073709551615", "18446744073", 2);
+  test_d2ll("18446744073709551615", "9223372036854775807", 2);
   test_d2ll("-1", "-1", 0);
   test_d2ll("-1.23", "-1", 1);
   test_d2ll("-9223372036854775807", "-9223372036854775807", 0);
@@ -3131,12 +3132,12 @@ int main()
 
   printf("==== decimal2string ====\n");
   test_pr("123.123", 0, 0, 0, "123.123", 0);
-  test_pr("123.123", 7, 3, '0', "123.123", 0);
-  test_pr("123.123", 9, 3, '0', "00123.123", 0);
-  test_pr("123.123", 9, 4, '0', "0123.1230", 0);
-  test_pr("123.123", 9, 5, '0', "123.12300", 0);
-  test_pr("123.123", 9, 2, '0', "000123.12", 1);
-  test_pr("123.123", 9, 6, '0', "23.123000", 2);
+  test_pr("123.123", 7, 3, '0', "0123.123", 0);
+  test_pr("123.123", 9, 3, '0', "000123.123", 0);
+  test_pr("123.123", 9, 4, '0', "00123.1230", 0);
+  test_pr("123.123", 9, 5, '0', "0123.12300", 0);
+  test_pr("123.123", 9, 2, '0', "0000123.12", 1);
+  test_pr("123.123", 8, 6, '0', "23.123000", 2);
 
   printf("==== decimal_shift ====\n");
   test_sh("123.123", 1, "1231.23", 0);