Hi, Michael! On Sep 08, Michael Widenius wrote: > revision-id: c79c6f78205 (mariadb-10.5.2-268-gc79c6f78205) > parent(s): bf8bd057344 > author: Michael Widenius <michael.widen...@gmail.com> > committer: Michael Widenius <michael.widen...@gmail.com> > timestamp: 2020-09-02 20:58:33 +0300 > message: > > 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);
I don't understand. Is it the type for precision (max number of digits in a decimal number) or for scale (mux number of digits after the dot)? If the former - why did you not change the type of the precision argument? If the latter - why is it called not decimal_scale_t? I personally think the former, using decimal_digits_t for both precision and scale, should be fine. > -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); > int decimal_result_size(decimal_t *from1, decimal_t *from2, char op, > int param); > > diff --git a/include/m_ctype.h b/include/m_ctype.h > index 59ac7814aee..34821f76474 100644 > --- a/include/m_ctype.h > +++ b/include/m_ctype.h > @@ -79,6 +79,7 @@ typedef const struct my_collation_handler_st > MY_COLLATION_HANDLER; > typedef const struct unicase_info_st MY_UNICASE_INFO; > typedef const struct uni_ctype_st MY_UNI_CTYPE; > typedef const struct my_uni_idx_st MY_UNI_IDX; > +typedef uint16 decimal_digits_t; A bit strange to see it here and not in decimal.h above. > > typedef struct unicase_info_char_st > { > diff --git a/plugin/type_inet/sql_type_inet.h > b/plugin/type_inet/sql_type_inet.h > index 8c42431ccaa..df74b91172a 100644 > --- a/plugin/type_inet/sql_type_inet.h > +++ b/plugin/type_inet/sql_type_inet.h > @@ -402,19 +402,19 @@ class Type_handler_inet6: public Type_handler > bool can_return_time() const override { return false; } > bool convert_to_binary_using_val_native() const override { return true; } > > - uint Item_time_precision(THD *thd, Item *item) const override > + decimal_digits_t Item_time_precision(THD *thd, Item *item) const override here you return the precision using the decimal_digits_t type. So it seems that it should apply to both precision and scale. > { > return 0; > } > - uint Item_datetime_precision(THD *thd, Item *item) const override > +decimal_digits_t Item_datetime_precision(THD *thd, Item *item) const override > { > return 0; > } > - uint Item_decimal_scale(const Item *item) const override > + decimal_digits_t Item_decimal_scale(const Item *item) const override > { > return 0; > } > - uint Item_decimal_precision(const Item *item) const override > + decimal_digits_t Item_decimal_precision(const Item *item) const override > { > /* > This will be needed if we ever allow cast from INET6 to DECIMAL. > diff --git a/sql/field.cc b/sql/field.cc > index f50ddec1c80..a5d00dd6fd5 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -3242,10 +3242,11 @@ Field *Field_decimal::make_new_field(MEM_ROOT *root, > TABLE *new_table, > ** Field_new_decimal > ****************************************************************************/ > > -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); shouldn't my_decimal_length_to_precision() return decimal_digits_t result? > - return MY_MIN(precision, DECIMAL_MAX_PRECISION); > + return (decimal_digits_t) MY_MIN(precision, DECIMAL_MAX_PRECISION); > } > > Field_new_decimal::Field_new_decimal(uchar *ptr_arg, > diff --git a/sql/item.h b/sql/item.h > index 4cdee637415..948bb85f253 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1644,14 +1644,14 @@ class Item: public Value_source, > return type_handler()->Item_decimal_precision(this); > } > /* Returns the number of integer part digits only */ > - inline int decimal_int_part() const > - { return my_decimal_int_part(decimal_precision(), decimals); } > + inline decimal_digits_t decimal_int_part() const > + { return (decimal_digits_t) my_decimal_int_part(decimal_precision(), > decimals); } again, shouldn't my_decimal_int_part() return decimal_digits_t? > /* > Returns the number of fractional digits only. > NOT_FIXED_DEC is replaced to the maximum possible number > of fractional digits, taking into account the data type. > */ > - uint decimal_scale() const > + decimal_digits_t decimal_scale() const > { > return type_handler()->Item_decimal_scale(this); > } > @@ -4856,7 +4856,7 @@ class Item_temporal_literal :public Item_literal > Item_literal(thd) > { > collation= DTCollation_numeric(); > - decimals= dec_arg; > + decimals= (decimal_digits_t) dec_arg; shouldnt dec_arg be decimal_digits_t? > cached_time= *ltime; > } > > diff --git a/sql/item_func.cc b/sql/item_func.cc > index 8a75d2c9946..805647d2a04 100644 > --- a/sql/item_func.cc > +++ b/sql/item_func.cc > @@ -2713,7 +2713,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, You don't need to cast uint16 to int explicitly > truncate ? TRUNCATE : HALF_UP) > 1))) > return decimal_value; > return 0; > diff --git a/sql/item_func.h b/sql/item_func.h > index 5b4acdce1c6..d5aa1477ff3 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -1369,10 +1369,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; you don't need a cast here > collation= DTCollation_numeric(); > fix_char_length(my_decimal_precision_to_length_no_truncation(len, dec, > > unsigned_flag)); > diff --git a/sql/my_decimal.cc b/sql/my_decimal.cc > index edf3e82de40..ac86ff71b64 100644 > --- a/sql/my_decimal.cc > +++ b/sql/my_decimal.cc > @@ -198,7 +198,8 @@ str_set_decimal(uint mask, const my_decimal *val, > E_DEC_OVERFLOW > */ > > -int my_decimal::to_binary(uchar *bin, int prec, int scale, uint mask) const > +int my_decimal::to_binary(uchar *bin, int prec, decimal_digits_t scale, again, shouldn't prec be decimal_digits_t too? > + uint mask) const > { > int err1= E_DEC_OK, err2; > my_decimal rounded; > @@ -329,7 +330,7 @@ my_decimal *date2my_decimal(const MYSQL_TIME *ltime, > my_decimal *dec) > } > > > -void my_decimal_trim(ulonglong *precision, uint *scale) > +void my_decimal_trim(ulonglong *precision, decimal_digits_t *scale) and here? > { > if (!(*precision) && !(*scale)) > { > diff --git a/sql/my_decimal.h b/sql/my_decimal.h > index 2db08bf01e3..9f7232f94b4 100644 > --- a/sql/my_decimal.h > +++ b/sql/my_decimal.h > @@ -50,7 +50,7 @@ typedef struct st_mysql_time MYSQL_TIME; > #define DECIMAL_MAX_FIELD_SIZE DECIMAL_MAX_PRECISION > > > -inline uint my_decimal_size(uint precision, uint scale) > +inline uint my_decimal_size(uint precision, decimal_digits_t scale) and here? (and in many places below, that I'll stop commenting on) > { > /* > Always allocate more space to allow library to put decimal point > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index 9e0f9b013c0..6569ea1a415 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -1197,11 +1198,12 @@ > Type_numeric_attributes::aggregate_numeric_attributes_decimal(Item **item, > uint nitems, > bool > unsigned_arg) > { > - int max_int_part= find_max_decimal_int_part(item, nitems); > + decimal_digits_t max_int_part= find_max_decimal_int_part(item, nitems); > decimals= find_max_decimals(item, nitems); > - int precision= MY_MIN(max_int_part + decimals, DECIMAL_MAX_PRECISION); > + decimal_digits_t precision= (decimal_digits_t) why do you need a cast here? > + MY_MIN(max_int_part + decimals, DECIMAL_MAX_PRECISION); > max_length= my_decimal_precision_to_length_no_truncation(precision, > - (uint8) decimals, > + decimals, > unsigned_flag); > } > > @@ -6910,20 +6912,20 @@ bool Type_handler_string_result:: > > /***************************************************************************/ > > -uint Type_handler::Item_time_precision(THD *thd, Item *item) const > +decimal_digits_t Type_handler::Item_time_precision(THD *thd, Item *item) > const > { > return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); note, no cast here (good) > } > > > -uint Type_handler::Item_datetime_precision(THD *thd, Item *item) const > +decimal_digits_t Type_handler::Item_datetime_precision(THD *thd, Item *item) > const > { > return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); and not here > } > > > -uint Type_handler_string_result::Item_temporal_precision(THD *thd, Item > *item, > - bool is_time) const > +decimal_digits_t Type_handler_string_result:: > +Item_temporal_precision(THD *thd, Item *item, bool is_time) const > { > StringBuffer<64> buf; > String *tmp; > @@ -6939,34 +6941,34 @@ uint > Type_handler_string_result::Item_temporal_precision(THD *thd, Item *item, > Datetime(thd, &status, tmp->ptr(), tmp->length(), tmp->charset(), > Datetime::Options(TIME_FUZZY_DATES, TIME_FRAC_TRUNCATE)). > is_valid_datetime())) > - return MY_MIN(status.precision, TIME_SECOND_PART_DIGITS); > - return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); > + return (decimal_digits_t) MY_MIN(status.precision, > TIME_SECOND_PART_DIGITS); > + return (decimal_digits_t) MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); and here again. why? (and below too, I'll stop commenting on casts now) > } > > diff --git a/sql/sql_type.h b/sql/sql_type.h > index a6d85b5bb47..7b1a4f730db 100644 > --- a/sql/sql_type.h > +++ b/sql/sql_type.h > @@ -2955,9 +2955,10 @@ class Type_numeric_attributes > class Type_temporal_attributes: public Type_numeric_attributes > { > public: > - Type_temporal_attributes(uint int_part_length, uint dec, bool unsigned_arg) > + Type_temporal_attributes(uint32 int_part_length, decimal_digits_t dec, > bool unsigned_arg) > :Type_numeric_attributes(int_part_length + (dec ? 1 : 0), > - MY_MIN(dec, TIME_SECOND_PART_DIGITS), > + MY_MIN(dec, > + (decimal_digits_t) > TIME_SECOND_PART_DIGITS), this should not be necessary > unsigned_arg) > { > max_length+= decimals; Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp