Hi Øystein,
Thank you very much for your review!
I am sorry for delay with this task.
It would be easier for you to do another
round soon after this review. But I had to switch
to other tasks with more ugrent deadlines :(
I'm attaching a new patch, which includes your suggestions.
Please find my comments inline:
Øystein Grøvlen wrote:
> Hi Bar,
>
> This is looking good. My comments are inline.
>
> Sorry it took so long,
>
> Øystein
>
> Alexander Barkov wrote:
> > #At file:///home/bar/mysql-bzr/mysql-azalea.w2649/ based on
> revid:kostja@stripped
> >
> > 2862 Alexander Barkov 2009-08-20
> > WL#2649 Number-to-string conversions
> > @ mysql-test/r/bigint.result
> > Fixing tests
> > @ mysql-test/r/case.result
> > Fixing tests
> > @ mysql-test/r/ctype_cp1251.result
> > Adding tests
> > @ mysql-test/r/ctype_latin1.result
> > Adding tests
> > @ mysql-test/r/ctype_ucs.result
> > Adding tests
> > @ mysql-test/r/explain.result
> > Fixing tests
> > @ mysql-test/r/func_gconcat.result
> > Fixing tests
> > @ mysql-test/r/func_str.result
> > Fixing tests
> > @ mysql-test/r/metadata.result
> > Fixing tests
> > @ mysql-test/r/ps_1general.result
> > Fixing tests
> > @ mysql-test/r/ps_2myisam.result
> > Fixing tests
> > @ mysql-test/r/ps_3innodb.result
> > Fixing tests
> > @ mysql-test/r/ps_4heap.result
> > Fixing tests
> > @ mysql-test/r/ps_5merge.result
> > Fixing tests
> > @ mysql-test/r/show_check.result
> > Fixing tests
> > @ mysql-test/r/type_datetime.result
> > Fixing tests
> > @ mysql-test/r/type_varchar.result
> > Fixing tests
> > @ mysql-test/r/union.result
> > Fixing tests
> > @ mysql-test/suite/ndb/r/ps_7ndb.result
> > Fixing tests
> > @ mysql-test/t/ctype_cp1251.test
> > Adding tests
> > @ mysql-test/t/ctype_latin1.test
> > Adding tests
> > @ mysql-test/t/ctype_ucs.test
> > Adding tests
> > @ mysql-test/t/func_str.test
> > Fixing tests
> > @ sql/field.cc
> > - Return str result using my_charset_numeric.
> > - Using real multi-byte aware str_to_XXX functions.
> > @ sql/field.h
> > - Changing derivation of non-string field types to
> DERIVATION_NUMERIC.
> > - Changing binary() for numeric/datetime fields to always
> > return TRUE even if charset is not my_charset_bin. We need
> > this to keep ha_base_keytype() return HA_KEYTYPE_BINARY.
> > @ sql/item.cc
> > - Implementing generic val_str_ascii().
> > - Fixing Item_num::safe_charset_converter().
> > Previously it alligned binary string to
> > character string (for example by adding leading 0x00
> > when doing binary->UCS2 conversion). Now it just
> > converts from my_charset_numbner to "tocs".
> > - Other misc changes
> > @ sql/item.h
> > - Adding MY_COLL_ALLOW_NUMBER_CONV -
> > a new character set and collation aggregation flag
> > which forces conversion of numeric/datetime data types
> > to @@character_set_connection.
> > - Changing MY_COLL_CMP_CONV and MY_COLL_ALLOW_CONV to
> > bit operations instead of hard-coded bit masks.
> > - Addding new method DTCollation.set_numeric().
> > - Adding new methods to Item.
> > - Adding helper functions
> > - Changing charset for Item_num-derived items
> > from my_charset_bin to my_charset_numeric (an alias for
> latin1).
> > @ sql/item_cmpfunc.cc
> > - Using new helper functions
> > - Other misc changes
> > @ sql/item_cmpfunc.h
> > Fixing strcmp() to return max_length=2.
> > Previously it returned 1, which was wrong,
> > because it did not fit '-1'.
> > @ sql/item_func.cc
> > - Using new helper functions
> > - Other minor changes
> > @ sql/item_func.h
> > - Removing unused functions
> > - Adding helper functions
> > - Adding set_numeric() into constructors of numeric items.
> > - Using fix_length_and_charset() instead of direct write to
> max_length.
> > @ sql/item_geofunc.cc
> > - Changing class for Item_func_geometry_type and
> > Item_func_as_wkt from Item_str_func to
> > Item_str_ascii_func.
> > @ sql/item_geofunc.h
> > - Changing class for Item_func_geometry_type and
> > Item_func_as_wkt from Item_str_func to
> > Item_str_ascii_func. We need this to make these
> > functions return correct result when
> > @@character_set_connection is real-multibyte (like UCS2).
> > Previosly these functions did not work with UCS2 fine.
> > @ sql/item_strfunc.cc
> > - Adding val_str method for Item_str_ascii_func.
> > @ sql/item_strfunc.h
> > - Introducing Item_str_ascii_func - for functions
> > which return pure ASCII data. For performance purposes,
> > as well as for cases when the old implementation
> > of val_str() was heavily 8-bit oriented.
> > @ sql/item_sum.cc
> > Using new helper functions.
> > @ sql/item_timefunc.cc
> > - Changing base class for datetime functions to
> Item_str_ascii_func.
> > The old implementation of val_str() was heavily 8-bit oriented
> > and did not work with UCS2 well.
> > - Changing charset of these functions from my_charset_bin to
> > my_charset_number (an alias for latin1).
> > @ sql/item_timefunc.h
> > - Changing base class for datetime functions to
> Item_str_ascii_func.
> > The old implementation of val_str() was heavily 8-bit oriented
> > and did not work with UCS2 well.
> > - Changing charset of these functions from my_charset_bin to
> > my_charset_number (an alias for latin1).
> > @ sql/item_xmlfunc.cc
> > - Using new helper function
> > @ sql/my_decimal.cc
> > - Adding a new DECIMAL to CHAR converter
> > with real multibyte support (e.g. UCS2)
> > @ sql/mysql_priv.h
> > - Introducing a new derivation level for numeric/datetime
> data types.
> > - Adding macros for my_charset_numeric and
> MY_REPERTOIRE_NUMERIC.
> > - Adding prototypes for str_set_decimal()
> > - Adding prototypes for character-set aware str_to_xxx()
> functions.
> > @ sql/sql_select.cc
> > - Fixing a problem in optimizer, which was revealed by changing
> > character set of date/time data types from my_charset_bin to
> > my_charset_number. Don't allow joining of two string constants
> > when working with DATE, NEWDATE, TIME, DATETIME data types.
> > These types accept very different input format, so string
> literal
> > comparision is not possible.
> > @ sql/time.cc
> > - Adding to_ascii() helper function, to convert a string
> > in any character set to ascii representation. In the
> > future can be extended to understand digits written
> > in various non-Latin word scripts.
> > - Adding real multy-byte character set aware versions for
> str_to_XXXX,
> > to make these these type of queries work correct:
> >
> > INSERT INTO t1 SET datetime_column=ucs2_expression;
> >
> > modified:
> > mysql-test/r/bigint.result
> > mysql-test/r/case.result
> > mysql-test/r/ctype_cp1251.result
> > mysql-test/r/ctype_latin1.result
> > mysql-test/r/ctype_ucs.result
> > mysql-test/r/explain.result
> > mysql-test/r/func_gconcat.result
> > mysql-test/r/func_str.result
> > mysql-test/r/metadata.result
> > mysql-test/r/ps_1general.result
> > mysql-test/r/ps_2myisam.result
> > mysql-test/r/ps_3innodb.result
> > mysql-test/r/ps_4heap.result
> > mysql-test/r/ps_5merge.result
> > mysql-test/r/show_check.result
> > mysql-test/r/type_datetime.result
> > mysql-test/r/type_varchar.result
> > mysql-test/r/union.result
> > mysql-test/suite/ndb/r/ps_7ndb.result
> > mysql-test/t/ctype_cp1251.test
> > mysql-test/t/ctype_latin1.test
> > mysql-test/t/ctype_ucs.test
> > mysql-test/t/func_str.test
> > sql/field.cc
> > sql/field.h
> > sql/item.cc
> > sql/item.h
> > sql/item_cmpfunc.cc
> > sql/item_cmpfunc.h
> > sql/item_func.cc
> > sql/item_func.h
> > sql/item_geofunc.cc
> > sql/item_geofunc.h
> > sql/item_strfunc.cc
> > sql/item_strfunc.h
> > sql/item_sum.cc
> > sql/item_timefunc.cc
> > sql/item_timefunc.h
> > sql/item_xmlfunc.cc
> > sql/my_decimal.cc
> > sql/mysql_priv.h
> > sql/sql_select.cc
> > sql/time.cc
>
> ...
>
> > === modified file 'mysql-test/t/ctype_cp1251.test'
> > --- a/mysql-test/t/ctype_cp1251.test 2008-11-01 04:50:21 +0000
> > +++ b/mysql-test/t/ctype_cp1251.test 2009-08-20 10:18:05 +0000
> > @@ -58,3 +58,13 @@ SHOW CREATE TABLE t1;
> > DROP TABLE t1;
> >
> > # End of 4.1 tests
> > +
> > +--echo #
> > +--echo # Start of 5.4 tests
> > +--echo #
>
> I would guess it is not possible to say for certain which version this
> will end up in.
Fixed for 5.6 in the current patch. But the final version is still unknown.
>
> > +
> > +--source include/ctype_numconv.inc
>
> Did you forget to include this file in the patch?
Yes. Now included into the patch.
>
> ...
>
> >
> >
> > === modified file 'sql/field.h'
> > --- a/sql/field.h 2009-07-10 12:26:38 +0000
> > +++ b/sql/field.h 2009-08-20 10:18:05 +0000
> > @@ -644,6 +644,7 @@ public:
> > const char *field_name_arg,
> > uint8 dec_arg, bool zero_arg, bool unsigned_arg);
> > Item_result result_type () const { return REAL_RESULT; }
> > + enum Derivation derivation(void) const { return DERIVATION_NUMERIC; }
> > void prepend_zeros(String *value);
> > void add_zerofill_and_unsigned(String &res) const;
> > friend class Create_field;
> > @@ -1207,6 +1208,8 @@ public:
> > enum_field_types type() const { return MYSQL_TYPE_TIMESTAMP;}
> > enum ha_base_keytype key_type() const { return
> HA_KEYTYPE_ULONG_INT; }
> > enum Item_result cmp_type () const { return INT_RESULT; }
> > + enum Derivation derivation(void) const { return DERIVATION_NUMERIC; }
> > + bool binary() const { return 1; }
> > int store(const char *to,uint length,CHARSET_INFO *charset);
> > int store(double nr);
> > int store(longlong nr, bool unsigned_val);
> > @@ -1307,6 +1310,8 @@ public:
> > enum_field_types type() const { return MYSQL_TYPE_DATE;}
> > enum ha_base_keytype key_type() const { return
> HA_KEYTYPE_ULONG_INT; }
> > enum Item_result cmp_type () const { return INT_RESULT; }
> > + enum Derivation derivation(void) const { return DERIVATION_NUMERIC; }
> > + bool binary() const { return 1; }
> > int store(const char *to,uint length,CHARSET_INFO *charset);
> > int store(double nr);
> > int store(longlong nr, bool unsigned_val);
> > @@ -1352,6 +1357,8 @@ public:
> > enum_field_types real_type() const { return MYSQL_TYPE_NEWDATE; }
> > enum ha_base_keytype key_type() const { return HA_KEYTYPE_UINT24; }
> > enum Item_result cmp_type () const { return INT_RESULT; }
> > + enum Derivation derivation(void) const { return DERIVATION_NUMERIC; }
> > + bool binary() const { return 1; }
> > int store(const char *to,uint length,CHARSET_INFO *charset);
> > int store(double nr);
> > int store(longlong nr, bool unsigned_val);
> > @@ -1387,6 +1394,8 @@ public:
> > enum_field_types type() const { return MYSQL_TYPE_TIME;}
> > enum ha_base_keytype key_type() const { return HA_KEYTYPE_INT24; }
> > enum Item_result cmp_type () const { return INT_RESULT; }
> > + enum Derivation derivation(void) const { return DERIVATION_NUMERIC; }
> > + bool binary() const { return 1; }
> > int store_time(MYSQL_TIME *ltime, timestamp_type type);
> > int store(const char *to,uint length,CHARSET_INFO *charset);
> > int store(double nr);
> > @@ -1424,6 +1433,8 @@ public:
> > enum ha_base_keytype key_type() const { return
> HA_KEYTYPE_ULONGLONG; }
> > #endif
> > enum Item_result cmp_type () const { return INT_RESULT; }
> > + enum Derivation derivation(void) const { return DERIVATION_NUMERIC; }
> > + bool binary() const { return 1; }
> > uint decimals() const { return DATETIME_DEC; }
> > int store(const char *to,uint length,CHARSET_INFO *charset);
> > int store(double nr);
> >
> > === modified file 'sql/item.cc'
> > --- a/sql/item.cc 2009-07-23 12:21:41 +0000
> > +++ b/sql/item.cc 2009-08-20 10:18:05 +0000
> > @@ -201,6 +201,34 @@ bool Item::val_bool()
> > }
> >
> >
> > +/*
> > + For the items which don't have its own fast val_str_ascii()
> > + implementation we provide the slow version, which converts
> > + from the Item character set to ASCII.
> > +*/
> > +String *Item::val_str_ascii(String *str)
> > +{
> > + DBUG_ASSERT(fixed == 1);
> > +
> > + if (!(collation.collation->state & MY_CS_NONASCII))
> > + return val_str(str);
> > +
> > + DBUG_ASSERT(str != &str_value);
> > +
> > + uint errors;
> > + String *res= val_str(&str_value);
> > + if (!res)
> > + return 0;
> > +
> > + if ((null_value= str->copy(res->ptr(), res->length(),
> > + collation.collation, &my_charset_latin1,
> > + &errors)))
> > + return 0;
> > +
> > + return str;
> > +}
> > +
> > +
> > String *Item::val_string_from_real(String *str)
> > {
> > double nr= val_real();
> > @@ -442,10 +470,11 @@ uint Item::decimal_precision() const
> > if ((restype == DECIMAL_RESULT) || (restype == INT_RESULT))
> > {
> > uint prec=
> > - my_decimal_length_to_precision(max_length, decimals,
> unsigned_flag);
> > + my_decimal_length_to_precision(max_char_length(), decimals,
> > + unsigned_flag);
> > return min(prec, DECIMAL_MAX_PRECISION);
> > }
> > - return min(max_length, DECIMAL_MAX_PRECISION);
> > + return min(max_char_length(), DECIMAL_MAX_PRECISION);
> > }
> >
>
> ...
>
> >
> > @@ -795,44 +824,29 @@ Item *Item::safe_charset_converter(CHARS
> > */
> > Item *Item_num::safe_charset_converter(CHARSET_INFO *tocs)
> > {
>
> I have a feeling the comment above this method needs to be updated.
>
> ...
>
> > @@ -1940,13 +1959,19 @@ void Item_field::set_field(Field *field_
> > field=result_field=field_par; // for easy coding with
> fields
> > maybe_null=field->maybe_null();
> > decimals= field->decimals();
> > - max_length= field_par->max_display_length();
> > table_name= *field_par->table_name;
> > field_name= field_par->field_name;
> > db_name= field_par->table->s->db.str;
> > alias_name_used= field_par->table->alias_name_used;
> > unsigned_flag=test(field_par->flags & UNSIGNED_FLAG);
> > - collation.set(field_par->charset(), field_par->derivation());
> > + collation.set(field_par->derivation() == DERIVATION_NUMERIC ?
> > + &my_charset_numeric : field_par->charset(),
> > + field_par->derivation(),
> > + field_par->derivation() == DERIVATION_NUMERIC ?
> > + MY_REPERTOIRE_ASCII : MY_REPERTOIRE_UNICODE30);
> > + max_length= field_par->max_display_length() *
> > + (field_par->derivation() == DERIVATION_NUMERIC ?
> > + collation.collation->mbmaxlen : 1);
>
> I think this would be have been easier to read if written like
> this:
>
> if (field_par->derivation() == DERIVATION_NUMERIC)
> {
> collation.set(...);
> max_length= ...
> }
> else
> {
> collation.set(...);
> max_length= ...
> }
>
> By the way, did you consider implementing this logic in
> Field::charset() instead, or is the current behvior needed in other
> scenarios?
Thanks for the good suggestion!
I fixed Field::charset(), and also added Field::repertoire().
Now these two operations look very nice:
collation.set(field_par->charset(), field_par->derivation(),
field_par->repertoire());
fix_char_length(field_par->char_length());
>
> > fixed= 1;
> > if (field->table->s->tmp_table == SYSTEM_TMP_TABLE)
> > any_privileges= 0;
> > @@ -2268,7 +2293,7 @@ String *Item_int::val_str(String *str)
> > {
> > // following assert is redundant, because fixed=1 assigned in
> constructor
> > DBUG_ASSERT(fixed == 1);
> > - str->set(value, &my_charset_bin);
> > + str->set(value, collation.collation);
> > return str;
> > }
>
> Don't you have to do the same for Item_uint::val_str?
Thanks for noticing this!
Made the same for Item_uint, and added a text coverting Item_uint
(UNSIGNED BIGINT(20) in terms of SQL).
>
> ...
>
>
> >
> > @@ -2398,7 +2423,7 @@ double Item_decimal::val_real()
> >
> > String *Item_decimal::val_str(String *result)
> > {
> > - result->set_charset(&my_charset_bin);
> > + result->set_charset(&my_charset_numeric);
> > my_decimal2string(E_DEC_FATAL_ERROR, &decimal_value, 0, 0, 0,
> result);
> > return result;
> > }
> >
> > === modified file 'sql/item.h'
> > --- a/sql/item.h 2009-05-25 10:10:18 +0000
> > +++ b/sql/item.h 2009-08-20 10:18:05 +0000
>
> ...
>
> > @@ -1051,6 +1133,15 @@ public:
> > { return Field::GEOM_GEOMETRY; };
> > String *check_well_formed_result(String *str, bool send_error= 0);
> > bool eq_by_collation(Item *item, bool binary_cmp, CHARSET_INFO *cs);
> > + uint32 max_char_length() const
> > + { return max_length / collation.collation->mbmaxlen; }
>
> I guess this is OK even if it is not correct from a mathematical point
> of view. Generally, if some characters are encoded in less than
> mbmaxlen bytes,
> max_char_length may be greater than "max_length/mbmaxlen", but I guess
> the point here is that same mbmaxlen was used to compute max_length
> from max_char_length in the first place.
Your assumption is correct. For example, Item_string
does this as follows:
max_length= str_value.numchars()*cs->mbmaxlen;
>
> ...
>
> > --- a/sql/item_func.cc 2009-07-28 22:45:02 +0000
> > +++ b/sql/item_func.cc 2009-08-20 10:18:05 +0000
>
> ...
>
> > @@ -4033,9 +4043,15 @@ void
> > Item_func_set_user_var::fix_length_and_dec()
> > {
> > maybe_null=args[0]->maybe_null;
> > - max_length=args[0]->max_length;
> > decimals=args[0]->decimals;
> > - collation.set(args[0]->collation.collation, DERIVATION_IMPLICIT);
> > + collation.set(DERIVATION_IMPLICIT);
> > + if (args[0]->collation.derivation == DERIVATION_NUMERIC)
> > + fix_length_and_charset(args[0]->max_char_length(),
> default_charset());
> > + else
> > + {
> > + collation.set(args[0]->collation.collation);
> > + max_length= args[0]->max_length;
> > + }
> > }
>
> This looks a bit inconsistent. Why don't use fix_length_and_charset
> in the else part?
Fixed to this:
else
{
fix_length_and_charset(args[0]->max_char_length(),
args[0]->collation.collation);
}
>
> ...
>
> > === modified file 'sql/item_func.h'
> > --- a/sql/item_func.h 2009-07-15 11:11:12 +0000
> > +++ b/sql/item_func.h 2009-08-20 10:18:05 +0000
>
> ...
>
> > @@ -314,13 +316,18 @@ class Item_num_op :public Item_func_numh
> > class Item_int_func :public Item_func
> > {
> > public:
> > - Item_int_func() :Item_func() { max_length= 21; }
> > - Item_int_func(Item *a) :Item_func(a) { max_length= 21; }
> > - Item_int_func(Item *a,Item *b) :Item_func(a,b) { max_length= 21; }
> > + Item_int_func() :Item_func()
> > + { collation.set_numeric(); fix_char_length(21); }
> > + Item_int_func(Item *a) :Item_func(a)
> > + { collation.set_numeric(); fix_char_length(21); }
> > + Item_int_func(Item *a,Item *b) :Item_func(a,b)
> > + { collation.set_numeric(); fix_char_length(21); }
>
> Why not use fix_length_and_charset() here? Or maybe it is better to
> just drop that method and do this in two statements everywhere?
set_numeric() does more than just setting character set:
void set_numeric()
{
collation= &my_charset_numeric;
derivation= DERIVATION_NUMERIC;
repertoire= MY_REPERTOIRE_NUMERIC;
}
So if I used fix_length_and_charset(), I would
also need to set derivation and repertoire separately.
I don't think it's better to drop that method.
It makes the code shorter.
>
> ...
>
> > @@ -379,6 +391,7 @@ public:
> > decimals= dec;
> > max_length= my_decimal_precision_to_length_no_truncation(len, dec,
> > unsigned_flag);
> > + collation.set_numeric();
> > }
>
> I would prefer that you either used fix_char_length here or added a
> comment saying that max_length can be set directly because numeric
> charset implies one byte per character or something. For consistency,
> I would set collation before setting max_length like other places.
I agree. I fixed this piece.
>
> ...
>
> > === modified file 'sql/item_strfunc.cc'
> > --- a/sql/item_strfunc.cc 2009-07-06 08:38:21 +0000
> > +++ b/sql/item_strfunc.cc 2009-08-20 10:18:05 +0000
>
> ...
>
> > @@ -3352,11 +3375,11 @@ String* Item_func_inet_ntoa::val_str(Str
> > num[0]=(char) n1+'0';
> > num[1]=(char) n2+'0';
> > num[2]=(char) c+'0';
> > - uint length=(n1 ? 4 : n2 ? 3 : 2); // Remove pre-zero
> > -
> > - (void) str->append(num+4-length,length);
> > + uint length=(n1 ? 4 : n2 ? 3 : 2); // Remove pre-zero
>
> Missing space after =
Fixed.
>
> ...
>
> > === modified file 'sql/item_timefunc.h'
> > --- a/sql/item_timefunc.h 2009-05-18 05:14:37 +0000
> > +++ b/sql/item_timefunc.h 2009-08-20 10:18:05 +0000
> > @@ -92,20 +92,19 @@ public:
> > class Item_func_month :public Item_func
> > {
> > public:
> > - Item_func_month(Item *a) :Item_func(a) {}
> > + Item_func_month(Item *a) :Item_func(a) { collation.set_numeric(); }
> > longlong val_int();
> > double val_real()
> > { DBUG_ASSERT(fixed == 1); return (double)
> Item_func_month::val_int(); }
> > String *val_str(String *str)
> > {
> > - str->set(val_int(), &my_charset_bin);
> > + str->set(val_int(), collation.collation);
> > return null_value ? 0 : str;
> > }
> > const char *func_name() const { return "month"; }
> > enum Item_result result_type () const { return INT_RESULT; }
> > void fix_length_and_dec()
> > {
> > - collation.set(&my_charset_bin);
> > decimals=0;
> > max_length=2*MY_CHARSET_BIN_MB_MAXLEN;
> > maybe_null=1;
>
> I am bit concerned about assigning directly to max_length, especially
> when the setting of charset is moved somewhere else (e.g, the
> constructor). The risk is that someone later may change charset
> in the constructor without recognizing that fix_length_and_dec assumes
> a specific charset. Would it not be better to use fix_char_length
> here?
Fixed to:
fix_char_length(2 * MY_CHARSET_BIN_MB_MAXLEN);
>
> ...
>
> > @@ -734,12 +730,11 @@ public:
> > class Item_typecast_maybe_null :public Item_typecast
> > {
> > public:
> > - Item_typecast_maybe_null(Item *a) :Item_typecast(a) {}
> > + Item_typecast_maybe_null(Item *a) :Item_typecast(a) { maybe_null=
> 1; }
> > void fix_length_and_dec()
> > {
> > collation.set(&my_charset_bin);
> > max_length=args[0]->max_length;
> > - maybe_null= 1;
> > }
>
> Do you really need this method anymore? It seems to be identical to
> Item_typecast::fix_length_and_dec.
Thanks for noticing! Removed.
> Btw, why do you move the setting of maybe_null to the constructor
> for this class, but not for Item_func_sec_to_time?
Because Item_func_sec_to_time is not affected by WL otherwise.
To make this change all around the code, it should be a separate
refactoring WL.
>
> > === modified file 'sql/my_decimal.cc'
> > --- a/sql/my_decimal.cc 2008-11-27 16:44:19 +0000
> > +++ b/sql/my_decimal.cc 2009-08-20 10:18:05 +0000
> > @@ -110,10 +110,56 @@ int my_decimal2string(uint mask, const m
> > &length, (int)fixed_prec, fixed_dec,
> > filler);
> > str->length(length);
> > + str->set_charset(&my_charset_numeric);
> > return check_result(mask, result);
> > }
> >
> >
> > +/**
> > + @brief Converting decimal to string with character set conversion
> > +
> > + @details Convert given my_decimal to String; allocate buffer as
> needed.
> > +
> > + @param[in] mask what problems to warn on (mask of E_DEC_*
> values)
> > + @param[in] val the decimal to print
> > + @param[in] fixed_prec overall number of digits if ZEROFILL, 0
> otherwise
> > + @param[in] fixed_dec number of decimal places (if fixed_prec
> != 0)
> > + @param[in] filler what char to pad with (ZEROFILL et al.)
> > + @param[out] *str where to store the resulting string
> > + @param[in] cs character set
> > +
> > + @return error coce
> > + @retval E_DEC_OK
> > + @retval E_DEC_TRUNCATED
> > + @retval E_DEC_OVERFLOW
> > + @retval E_DEC_OOM
> > +
> > + Would be great to make it a method of the String class,
> > + but this would need to include
> > + my_decimal.h from sql_string.h and sql_string.cc, which is not
> desirable.
> > +*/
> > +bool
> > +str_set_decimal(uint mask, const my_decimal *val,
> > + uint fixed_prec, uint fixed_dec, char filler,
> > + String *str, CHARSET_INFO *cs)
> > +{
> > + if (!(cs->state & MY_CS_NONASCII))
> > + {
> > + my_decimal2string(mask, val, fixed_prec, fixed_dec, filler, str);
> > + str->set_charset(cs);
> > + return FALSE;
> > + }
> > + else
> > + {
> > + uint errors;
> > + char buf[DECIMAL_MAX_STR_LENGTH];
> > + String tmp(buf, sizeof(buf), &my_charset_latin1);
> > + my_decimal2string(mask, val, fixed_prec, fixed_dec, filler, &tmp);
> > + return str->copy(tmp.ptr(), tmp.length(), &my_charset_latin1,
> cs, &errors);
> > + }
> > +}
>
>
> Could you add a comment that states the rationale for the different
> handling in if and else part?
Added.
>
>
> > +
> > +
> > /*
> > Convert from decimal to binary representation
> >
> >
> > === modified file 'sql/mysql_priv.h'
> > --- a/sql/mysql_priv.h 2009-08-14 19:52:00 +0000
> > +++ b/sql/mysql_priv.h 2009-08-20 10:18:05 +0000
>
> Note that there is ongoing work by Mats to split file. If possibly,
> you should try to avoid to add new stuff to this file.
Ok.
>
>
> > @@ -175,7 +175,8 @@ extern CHARSET_INFO *error_message_chars
> >
> > enum Derivation
> > {
> > - DERIVATION_IGNORABLE= 5,
> > + DERIVATION_IGNORABLE= 6,
> > + DERIVATION_NUMERIC= 5,
> > DERIVATION_COERCIBLE= 4,
> > DERIVATION_SYSCONST= 3,
> > DERIVATION_IMPLICIT= 2,
> > @@ -183,6 +184,8 @@ enum Derivation
> > DERIVATION_EXPLICIT= 0
> > };
> >
> > +#define my_charset_numeric my_charset_latin1
> > +#define MY_REPERTOIRE_NUMERIC MY_REPERTOIRE_ASCII
>
> Could these defines go somewhere else?
Not sure. It could go to m_ctype.h.
But these defines are not needed on the client side.
So mysql_priv.h looks the best place.
>
> >
> > typedef struct my_locale_errmsgs
> > {
> > @@ -827,6 +830,16 @@ typedef Comp_creator* (*chooser_compare_
> > #include "item.h"
> > extern my_decimal decimal_zero;
> >
> > +/* my_decimal.cc */
> > +bool str_set_decimal(uint mask, const my_decimal *val, uint fixed_prec,
> > + uint fixed_dec, char filler, String *str,
> > + CHARSET_INFO *cs);
> > +inline bool str_set_decimal(const my_decimal *val, String *str,
> > + CHARSET_INFO *cs)
> > +{
> > + return str_set_decimal(E_DEC_FATAL_ERROR, val, 0, 0, 0, str, cs);
> > +}
> > +
>
> This method seems to be only used in one place. Do you really need to
> make it globally available?
Sorry, which one? str_set_decimal() is quite big.
my_decimal.cc looks the best place for it.
>
> > /* sql_parse.cc */
> > void free_items(Item *item);
> > void cleanup_items(Item *item);
> > @@ -2201,8 +2214,17 @@ ulong convert_month_to_period(ulong mont
> > void get_date_from_daynr(long daynr,uint *year, uint *month,
> > uint *day);
> > my_time_t TIME_to_timestamp(THD *thd, const MYSQL_TIME *t, my_bool
> *not_exist);
> > -bool str_to_time_with_warn(const char *str,uint length,MYSQL_TIME
> *l_time);
> > -timestamp_type str_to_datetime_with_warn(const char *str, uint length,
> > +/* Character set-aware version of str_to_time() */
> > +bool str_to_time(CHARSET_INFO *cs, const char *str,uint length,
> > + MYSQL_TIME *l_time, int *warning);
> > +/* Character set-aware version of str_to_datetime() */
> > +timestamp_type str_to_datetime(CHARSET_INFO *cs,
> > + const char *str, uint length,
> > + MYSQL_TIME *l_time, uint flags, int
> *was_cut);
> > +bool str_to_time_with_warn(CHARSET_INFO *cs, const char *str,uint
> length,
> > + MYSQL_TIME *l_time);
> > +timestamp_type str_to_datetime_with_warn(CHARSET_INFO *cs,
> > + const char *str, uint length,
> > MYSQL_TIME *l_time, uint
> flags);
> > void localtime_to_TIME(MYSQL_TIME *to, struct tm *from);
> > void calc_time_from_sec(MYSQL_TIME *to, long seconds, long
> microseconds);
>
> I guess the time stuff has to go here for now. Mats is moving it to a
> new file, sql_time.h, so there will be conflicts anyway.
Ok.
>
>
> >
> > === modified file 'sql/sql_select.cc'
> > --- a/sql/sql_select.cc 2009-07-28 14:16:37 +0000
> > +++ b/sql/sql_select.cc 2009-08-20 10:18:05 +0000
> > @@ -12080,6 +12080,69 @@ static void update_const_equal_items(CON
> >
> >
> > /*
> > + Check if
> > + field=const_a AND field=const_b
> > + can be changed to
> > + field=const_a AND const_a=const_b
> > +
> > + This is not always possible:
> > +
> > + field_datetime='2001-02-03 00:00:00' AND
> field_datetime='2001-02-03'
> > +
> > + cannot be changed to
> > +
> > + field_datetime='2001-02-03 00:00:00' AND '2001-02-03
> 00:00:00'='2001-02-03'
> > +
> > + because '2001-02-03 00:00:00' will be compared to '2001-02-03'
> > + as string (instead of datetime) and we'll get "Impossible WHERE
> condition".
> > +
> > +*/
> > +static bool
> > +can_join_constants(Item *field, Item *a, Item *b)
> > +{
> > + if (a->result_type() != STRING_RESULT ||
> > + b->result_type() != STRING_RESULT)
> > + return TRUE;
> > +
> > + switch (field->field_type())
> > + {
> > + case MYSQL_TYPE_DECIMAL:
> > + case MYSQL_TYPE_NEWDECIMAL:
> > + case MYSQL_TYPE_TINY:
> > + case MYSQL_TYPE_SHORT:
> > + case MYSQL_TYPE_INT24:
> > + case MYSQL_TYPE_LONG:
> > + case MYSQL_TYPE_LONGLONG:
> > + case MYSQL_TYPE_FLOAT:
> > + case MYSQL_TYPE_DOUBLE:
> > + case MYSQL_TYPE_NULL:
> > + case MYSQL_TYPE_TIMESTAMP:
> > + case MYSQL_TYPE_YEAR:
> > + case MYSQL_TYPE_DATE:
> > + case MYSQL_TYPE_NEWDATE:
> > + case MYSQL_TYPE_TIME:
> > + case MYSQL_TYPE_DATETIME:
> > + return FALSE;
> > +
> > + case MYSQL_TYPE_VARCHAR:
> > + case MYSQL_TYPE_BLOB:
> > + case MYSQL_TYPE_GEOMETRY:
> > + case MYSQL_TYPE_STRING:
> > + case MYSQL_TYPE_BIT:
> > + case MYSQL_TYPE_ENUM:
> > + case MYSQL_TYPE_SET:
> > + case MYSQL_TYPE_TINY_BLOB:
> > + case MYSQL_TYPE_MEDIUM_BLOB:
> > + case MYSQL_TYPE_LONG_BLOB:
> > + case MYSQL_TYPE_VAR_STRING:
> > + case MAX_NO_FIELD_TYPES:
> > + break;
> > + }
> > + return a->collation.collation == b->collation.collation;
> > +}
> > +
> > +
> > +/*
> > change field = field to field = const for each found field = const
> in the
> > and_level
> > */
> > @@ -12111,9 +12174,7 @@ change_cond_ref_to_const(THD *thd, I_Lis
> >
> > if (right_item->eq(field,0) && left_item != value &&
> > right_item->cmp_context == field->cmp_context &&
> > - (left_item->result_type() != STRING_RESULT ||
> > - value->result_type() != STRING_RESULT ||
> > - left_item->collation.collation == value->collation.collation))
> > + can_join_constants(field, left_item, value))
> > {
> > Item *tmp=value->clone_item();
> > tmp->collation.set(right_item->collation);
> > @@ -12135,9 +12196,7 @@ change_cond_ref_to_const(THD *thd, I_Lis
> > }
> > else if (left_item->eq(field,0) && right_item != value
> &&
> > left_item->cmp_context == field->cmp_context &&
> > - (right_item->result_type() != STRING_RESULT ||
> > - value->result_type() != STRING_RESULT ||
> > - right_item->collation.collation ==
> value->collation.collation))
> > + can_join_constants(field, right_item, value))
> > {
> > Item *tmp= value->clone_item();
> > tmp->collation.set(left_item->collation);
> >
>
> How is the above related to the work log. Is it a bug fix? Could it
> be done as a separate patch?
I reporting the problem as a separate bug:
Bug#49199 Optimizer handles incorrectly: field='const1' AND
field='const2' in some cases.
Ramil has fixed the bug, and the fix is already in mysql-next-mr.
So I don't need this code any more.
>
> That's all.
Thank you very much!
Attachment: [application/x-gzip] w2649.diff.gz
