Bar,
I have so far only looked at the C++-part of the patch, not the tests.
(I will come back to them later.) I also have not tried to apply the
patch yet, which I think is necessary in order to figure out if
something is missing. Do you have any recommendation on which version
of which branch to apply the patch?
Most of the changes looks good, but I have a few questions/comments:
1. I think I understood most of the worklog, but I am a bit puzzled
with the following statement: "Now this worklog has nothing to do
with ascii." Why then does this patch introduce methods called
val_ascii and change charsets from binary to latin1? Is this
related to what is said in the section "Other automatic
conversions"? I am not really able to tell what that section is
saying. I think the worklog needs to explain this better.
2. Why do you rename methods from val_str to val_ascii? As far as I
understand, the val_xxx methods represents the four basic types
with operating on int, real, decimal, and string. You are not
adding another type here, just changing the representation of
strings. Why not just change val_str to return ascii where
appropriate? Do you ever need both an ascii and a non-ascii
representation of the same item value?
Note also that as part of the reenginering project, the val_xxx
methods will be disappear, and replaced with a value() method that
will return a Value object on which one can call type conversion
methods. (e.g., value().to_string()). See WL#4760.
3. I feel the use of set_char_length_and_dec is a bit inconsistent.
In some classes, decimals is set explicitly and set_char_length
called. In other places, set_char_length_and_dec is called with
two parameters. In yet other places, set_char_length is not used
at all. Instead, the multiplication is done without a function
call. Some consistency would be good.
If it were up to me, I would drop the entire
set_char_length_and_dec, initialize decimals to 0 in Item
constructors, and set decimals explicitly when necessary in
fix_lenght_and_dec etc. I also think it is a bit confusing to have
both fix_lenght_and_dec and set_char_length_and_dec methods. Not
easy to tell the difference from the name.
4. There is a lot of multiplication/division back and forth in
max_char_length and set_char_length. (By the way, should not the
latter be called set_max_char_length?). Did you consider storing
max_char_length, instead?
5. Please add some documentation on new methods (e.g.,
Item::val_ascii).
6. It is probably not my business, since I am not the architectural
reviewer, but it would be easier to verify the behavior if the work
log had contained a table with all functions affected, what they
used to return, what they will return, and what charset will be
used. The way it is now, it is a bit hard to verify that all has
been covered, and, especially, what charset should be used.
More detailed comments can be found in-line,
--
Øystein
Alexander Barkov wrote:
> #At file:///home/bar/mysql-bzr/mysql-6.0.w2649/
>
> 2701 Alexander Barkov 2009-03-04
> WL#2649 Number-to-string conversions
> added:
> mysql-test/include/ctype_numconv.inc
> mysql-test/r/ctype_binary.result
> mysql-test/t/ctype_binary.test
> modified:
> mysql-test/r/bigint.result
> mysql-test/r/case.result
> mysql-test/r/ctype_latin1.result
> mysql-test/r/ctype_ucs.result
> mysql-test/r/explain.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/union.result
> mysql-test/suite/ndb/r/ps_7ndb.result
> mysql-test/t/ctype_latin1.test
> mysql-test/t/ctype_ucs.test
> 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_timefunc.cc
> sql/item_timefunc.h
> sql/mysql_priv.h
> sql/sql_select.cc
> sql/time.cc
>
...
> === modified file 'sql/item.cc'
> --- a/sql/item.cc 2009-02-05 07:49:32 +0000
> +++ b/sql/item.cc 2009-03-04 06:59:35 +0000
> @@ -201,6 +201,27 @@ bool Item::val_bool()
> }
>
>
Some documentation on the purpose of this method would be good.
> +String *Item::val_ascii(String *str)
> +{
> + DBUG_ASSERT(fixed == 1);
> +
> + if (!(collation.collation->state & MY_CS_NONASCII))
> + return val_str(str);
> +
> + 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;
> +}
> +
> +
...
> @@ -912,8 +933,8 @@ bool Item::get_date(MYSQL_TIME *ltime,ui
> if (result_type() == STRING_RESULT)
> {
> char buff[40];
> - String tmp(buff,sizeof(buff), &my_charset_bin),*res;
> - if (!(res=val_str(&tmp)) ||
> + String tmp(buff, sizeof(buff), &my_charset_latin1), *res;
> + if (!(res= val_ascii(&tmp)) ||
> str_to_datetime_with_warn(res->ptr(), res->length(),
> ltime, fuzzydate) <=
MYSQL_TIMESTAMP_ERROR)
> goto err;
What is the reasoning behind dates not being dependent on connection
character set, but always be latin1?
...
> @@ -2258,8 +2279,8 @@ Item_decimal::Item_decimal(const char *s
> name= (char*) str_arg;
> decimals= (uint8) decimal_value.frac;
> fixed= 1;
> - max_length= my_decimal_precision_to_length(decimal_value.intg +
decimals,
> - decimals, unsigned_flag);
> + set_char_length(my_decimal_precision_to_length(decimal_value.intg
+ decimals,
> + decimals,
unsigned_flag));
> }
>
> Item_decimal::Item_decimal(longlong val, bool unsig)
> @@ -2267,8 +2288,8 @@ Item_decimal::Item_decimal(longlong val,
> int2my_decimal(E_DEC_FATAL_ERROR, val, unsig, &decimal_value);
> decimals= (uint8) decimal_value.frac;
> fixed= 1;
> - max_length= my_decimal_precision_to_length(decimal_value.intg +
decimals,
> - decimals, unsigned_flag);
> + set_char_length(my_decimal_precision_to_length(decimal_value.intg
+ decimals,
> + decimals,
unsigned_flag));
> }
>
>
> @@ -2277,8 +2298,8 @@ Item_decimal::Item_decimal(double val, i
> double2my_decimal(E_DEC_FATAL_ERROR, val, &decimal_value);
> decimals= (uint8) decimal_value.frac;
> fixed= 1;
> - max_length= my_decimal_precision_to_length(decimal_value.intg +
decimals,
> - decimals, unsigned_flag);
> + set_char_length(my_decimal_precision_to_length(decimal_value.intg
+ decimals,
> + decimals,
unsigned_flag));
> }
>
>
> @@ -2287,8 +2308,7 @@ Item_decimal::Item_decimal(const char *s
> {
> my_decimal2decimal(val_arg, &decimal_value);
> name= (char*) str;
> - decimals= (uint8) decimal_par;
> - max_length= length;
> + set_char_length_and_dec(length, decimal_par);
> fixed= 1;
> }
>
> @@ -2298,8 +2318,8 @@ Item_decimal::Item_decimal(my_decimal *v
> my_decimal2decimal(value_par, &decimal_value);
> decimals= (uint8) decimal_value.frac;
> fixed= 1;
> - max_length= my_decimal_precision_to_length(decimal_value.intg +
decimals,
> - decimals, unsigned_flag);
> + set_char_length(my_decimal_precision_to_length(decimal_value.intg
+ decimals,
> + decimals,
unsigned_flag));
> }
>
>
> @@ -2309,8 +2329,8 @@ Item_decimal::Item_decimal(const uchar *
> &decimal_value, precision, scale);
> decimals= (uint8) decimal_value.frac;
> fixed= 1;
> - max_length= my_decimal_precision_to_length(precision, decimals,
> - unsigned_flag);
> + set_char_length(my_decimal_precision_to_length(precision, decimals,
> + unsigned_flag));
> }
In the above Item_decimal constructors, you use two ways of setting
decimals and max_length:
1. Set decimals and call set_char_length
2. Call set_char_length_and_dec with two params
I think some consistency here would be good.
...
> @@ -531,19 +533,22 @@ void Item_func::fix_num_length_and_dec()
> for (uint i=0 ; i < arg_count ; i++)
> {
> set_if_bigger(decimals,args[i]->decimals);
> - set_if_bigger(fl_length, args[i]->max_length);
> + set_if_bigger(fl_length, args[i]->max_char_length());
> }
> - max_length=float_length(decimals);
> + max_length= float_length(decimals);
> if (fl_length > max_length)
> {
> decimals= NOT_FIXED_DEC;
> max_length= float_length(NOT_FIXED_DEC);
> }
> + set_char_length(max_length);
> }
I do not like this way of using max_length for the intermediate
results. This way, the unit of max_length is changed from #chars to
#bytes in the middle of this function. That is a bit confusing.
...
> @@ -599,9 +605,9 @@ void Item_func::count_real_length()
> if (decimals != NOT_FIXED_DEC)
> {
> set_if_bigger(decimals, args[i]->decimals);
> - set_if_bigger(length, (args[i]->max_length - args[i]->decimals));
> + set_if_bigger(length, (args[i]->max_char_length() -
args[i]->decimals));
> }
> - set_if_bigger(max_length, args[i]->max_length);
> + set_if_bigger(max_length, args[i]->max_char_length());
> }
> if (decimals != NOT_FIXED_DEC)
> {
> @@ -612,6 +618,7 @@ void Item_func::count_real_length()
> else
> max_length= length;
> }
> + max_length*= collation.collation->mbmaxlen;
> }
Why don't you use set_char_length here?
...
> @@ -686,7 +692,7 @@ void Item_num_op::find_num_type(void)
> r0 == STRING_RESULT || r1 ==STRING_RESULT)
> {
> count_real_length();
> - max_length= float_length(decimals);
> + max_length= float_length(decimals) * collation.collation->mbmaxlen;
> hybrid_type= REAL_RESULT;
> }
> else if (r0 == DECIMAL_RESULT || r1 == DECIMAL_RESULT)
> @@ -727,7 +733,7 @@ void Item_func_num1::find_num_type()
> case STRING_RESULT:
> case REAL_RESULT:
> hybrid_type= REAL_RESULT;
> - max_length= float_length(decimals);
> + max_length= float_length(decimals) * collation.collation->mbmaxlen;
> break;
> case DECIMAL_RESULT:
> break;
and here?
...
> @@ -757,6 +762,30 @@ void Item_func_numhybrid::fix_length_and
> }
>
>
> +/*
> + Would be greate to make it a method of the String class,
Spelling: greate
> + but this would need to include
> + my_decimal.h from sql_string.h and sql_string.cc, which is not
desirable.
And don't think you would need to include it in sql_string.h. Forward
declaration would be sufficient. (But I understand your point)
> +*/
> +bool
> +str_set_decimal(my_decimal *val, String *str, CHARSET_INFO *cs)
> +{
> + if (!(cs->state & MY_CS_NONASCII))
> + {
> + my_decimal2string(E_DEC_FATAL_ERROR, val, 0, 0, 0, str);
> + return FALSE;
> + }
> + else
> + {
> + uint errors;
> + char buf[DECIMAL_MAX_STR_LENGTH];
> + String tmp(buf, sizeof(buf), &my_charset_latin1);
> + my_decimal2string(E_DEC_FATAL_ERROR, val, 0, 0, 0, &tmp);
> + return str->copy(tmp.ptr(), tmp.length(), &my_charset_latin1,
cs, &errors);
> + }
> +}
As far as I can tell, this does not handle that the decimal separator
may different for different languages/locale (e.g., point vs. comma).
Is this not supported?
...
> @@ -1835,20 +1868,20 @@ longlong Item_func_bit_neg::val_int()
>
> void Item_func_integer::fix_length_and_dec()
> {
> - max_length=args[0]->max_length - args[0]->decimals+1;
> + max_length= args[0]->max_char_length() - args[0]->decimals + 1;
> uint tmp=float_length(decimals);
> set_if_smaller(max_length,tmp);
> - decimals=0;
> + set_char_length_and_dec(max_length);
> }
>
> void Item_func_int_val::fix_num_length_and_dec()
> {
> - max_length= args[0]->max_length - (args[0]->decimals ?
> - args[0]->decimals + 1 :
> - 0) + 2;
> + max_length= args[0]->max_char_length() - (args[0]->decimals ?
> + args[0]->decimals + 1 :
> + 0) + 2;
> uint tmp= float_length(decimals);
> set_if_smaller(max_length,tmp);
> - decimals= 0;
> + set_char_length_and_dec(max_length);
> }
Here it seems you are mixing char length and byte length. Is the
above really correct?
>
>
> @@ -1861,7 +1894,7 @@ void Item_func_int_val::find_num_type()
> case STRING_RESULT:
> case REAL_RESULT:
> hybrid_type= REAL_RESULT;
> - max_length= float_length(decimals);
> + max_length= float_length(decimals) * collation.collation->mbmaxlen;
> break;
> case INT_RESULT:
> case DECIMAL_RESULT:
> @@ -1993,10 +2026,11 @@ void Item_func_round::fix_length_and_dec
> longlong val1;
> bool val1_unsigned;
>
> + collation.set(default_charset(), DERIVATION_COERCIBLE,
MY_REPERTOIRE_ASCII);
Why is this needed here? I see a lot of use of collation without
having set it. I have not figured out how you know when it needs to
be set and when it has been set before.
> unsigned_flag= args[0]->unsigned_flag;
> if (!args[1]->const_item())
> {
> - max_length= args[0]->max_length;
> + max_length= args[0]->max_char_length();
> decimals= args[0]->decimals;
> if (args[0]->result_type() == DECIMAL_RESULT)
> {
> @@ -2005,6 +2039,7 @@ void Item_func_round::fix_length_and_dec
> }
> else
> hybrid_type= REAL_RESULT;
> + max_length*= collation.collation->mbmaxlen;
> return;
> }
More inconsistency: max_char_length() is used to get max_length, but
set_char_length() is not used to set it. I guess that explains why
collation.set was needed. Another call for consistency.
...
> @@ -3987,9 +4025,9 @@ void
> Item_func_set_user_var::fix_length_and_dec()
> {
> maybe_null=args[0]->maybe_null;
> + collation.set(args[0]->collation.collation, DERIVATION_IMPLICIT);
> max_length=args[0]->max_length;
> decimals=args[0]->decimals;
> - collation.set(args[0]->collation.collation, DERIVATION_IMPLICIT);
> }
Why did you need to move this statement?
By the way, why is not set_char_length needed here?
...
> @@ -5356,8 +5390,8 @@ longlong Item_func_inet_aton::val_int()
> char buff[36];
> int dot_count= 0;
>
> - String *s,tmp(buff,sizeof(buff),&my_charset_bin);
> - if (!(s = args[0]->val_str(&tmp))) // If null value
> + String *s, tmp(buff, sizeof(buff), &my_charset_latin1);
> + if (!(s = args[0]->val_ascii(&tmp))) // If null value
> goto err;
> null_value=0;
There is a comment "// Assume ascii" further down in the code that
becomes a bit strange now that we know that it is ascii.
>
>
> === modified file 'sql/item_func.h'
> --- a/sql/item_func.h 2009-01-23 19:37:08 +0000
> +++ b/sql/item_func.h 2009-03-04 06:59:35 +0000
> @@ -215,7 +215,10 @@ public:
> { DBUG_ASSERT(fixed == 1); return (longlong) rint(val_real()); }
> enum Item_result result_type () const { return REAL_RESULT; }
> void fix_length_and_dec()
> - { decimals= NOT_FIXED_DEC; max_length= float_length(decimals); }
> + {
> + decimals= NOT_FIXED_DEC;
> + set_char_length(float_length(NOT_FIXED_DEC));
> + }
> };
>
>
> @@ -324,7 +327,7 @@ public:
> double val_real();
> String *val_str(String*str);
> enum Item_result result_type () const { return INT_RESULT; }
> - void fix_length_and_dec() {}
> + void fix_length_and_dec() { set_char_length_and_dec(21); }
> };
>
Why do you need to set this when it is set in the constructor of the
parent class?
By the way, lot's of fix_length_and_dec functions uses hard-coded
constants. Why not use the constructor instead?
>
> @@ -349,7 +352,7 @@ public:
> longlong val_int();
> longlong val_int_from_str(int *error);
> void fix_length_and_dec()
> - { max_length=args[0]->max_length; unsigned_flag=0; }
> + { set_char_length_and_dec(args[0]->max_char_length());
unsigned_flag=0; }
Is this necessary because the charset of this and args[0] might be
different? Otherwise, the only thing this change does, is add one
extra multiplication and one extra division.
> virtual void print(String *str, enum_query_type query_type);
> uint decimal_precision() const { return
args[0]->decimal_precision(); }
> };
> @@ -362,7 +365,8 @@ public:
> const char *func_name() const { return "cast_as_unsigned"; }
> void fix_length_and_dec()
> {
> - max_length= min(args[0]->max_length, DECIMAL_MAX_PRECISION + 2);
> + set_char_length_and_dec(min(args[0]->max_char_length(),
> + DECIMAL_MAX_PRECISION + 2));
> unsigned_flag=1;
> }
> longlong val_int();
> @@ -385,7 +389,7 @@ public:
> my_decimal *val_decimal(my_decimal*);
> enum Item_result result_type () const { return DECIMAL_RESULT; }
> enum_field_types field_type() const { return MYSQL_TYPE_NEWDECIMAL; }
> - void fix_length_and_dec() {};
> + void fix_length_and_dec() { set_char_length(max_length); };
I do not understand this. This says that max_length is off by a
mbmaxlen factor. Why could it not be set correctly in the first
place?
...
> @@ -714,6 +715,7 @@ public:
> Item_func_sign(Item *a) :Item_int_func(a) {}
> const char *func_name() const { return "sign"; }
> longlong val_int();
> + void fix_length_and_dec() { set_char_length_and_dec(2); }
> };
This seems like a change of behavior. Is this fixing a bug?
...
> @@ -877,7 +877,7 @@ public:
> Item_func_ascii(Item *a) :Item_int_func(a) {}
> longlong val_int();
> const char *func_name() const { return "ascii"; }
> - void fix_length_and_dec() { max_length=3; }
> + void fix_length_and_dec() { set_char_length_and_dec(3); }
> };
This was not set before either. Is it some of your other changes that
makes this change necessary, or is it an old bug?
...
> @@ -1695,7 +1694,8 @@ public:
> Item_func_found_rows() :Item_int_func() {}
> longlong val_int();
> const char *func_name() const { return "found_rows"; }
> - void fix_length_and_dec() { decimals= 0; maybe_null=0; }
> + void fix_length_and_dec()
> + { Item_int_func::fix_length_and_dec(); maybe_null= 0; }
> };
>
>
> @@ -1707,8 +1707,7 @@ public:
> Item_func_uuid_short() :Item_int_func() {}
> const char *func_name() const { return "uuid_short"; }
> longlong val_int();
> - void fix_length_and_dec()
> - { max_length= 21; unsigned_flag=1; }
> + void fix_length_and_dec() { set_char_length_and_dec(21);
unsigned_flag= 1; }
Why is not Item_int_func::fix_length_and_dec() called here like for
the classes above?
> bool check_partition_func_processor(uchar *int_arg) {return FALSE;}
> };
>
>
> === modified file 'sql/item_geofunc.cc'
> --- a/sql/item_geofunc.cc 2007-12-21 20:10:05 +0000
> +++ b/sql/item_geofunc.cc 2009-03-04 06:59:35 +0000
...
> -String *Item_func_as_wkt::val_str(String *str)
> +String *Item_func_as_wkt::val_ascii(String *str)
> {
> DBUG_ASSERT(fixed == 1);
> String arg_val;
> @@ -122,6 +122,7 @@ String *Item_func_as_wkt::val_str(String
>
> void Item_func_as_wkt::fix_length_and_dec()
> {
> + collation.set(default_charset(), DERIVATION_COERCIBLE,
MY_REPERTOIRE_ASCII);
> max_length=MAX_BLOB_WIDTH;
> maybe_null= 1;
> }
Why is collation set to default_charset() when val_ascii is to be used
instead of val_str? And why do you not use set_char_length() to set the
collation and length?
> @@ -145,7 +146,7 @@ String *Item_func_as_wkb::val_str(String
> }
>
>
> -String *Item_func_geometry_type::val_str(String *str)
> +String *Item_func_geometry_type::val_ascii(String *str)
> {
> DBUG_ASSERT(fixed == 1);
> String *swkb= args[0]->val_str(str);
> @@ -158,8 +159,7 @@ String *Item_func_geometry_type::val_str
> return 0;
> /* String will not move */
> str->copy(geom->get_class_info()->m_name.str,
> - geom->get_class_info()->m_name.length,
> - default_charset());
> + geom->get_class_info()->m_name.length, &my_charset_latin1);
> return str;
> }
I cannot find anything in the worklog that says that the character set
for geometry_type should change. Where is that covered?
>
>
> === modified file 'sql/item_geofunc.h'
> --- a/sql/item_geofunc.h 2008-02-22 10:30:33 +0000
> +++ b/sql/item_geofunc.h 2009-03-04 06:59:35 +0000
...
> @@ -231,48 +231,60 @@ public:
> Item_func::print(str, query_type);
> }
>
> - void fix_length_and_dec() { maybe_null= 1; }
> + void fix_length_and_dec() { set_char_length_and_dec(2);
maybe_null= 1; }
> bool is_null() { (void) val_int(); return null_value; }
> };
>
> -class Item_func_isempty: public Item_bool_func
> +
> +class Item_geom_bool_func: public Item_bool_func
> {
> public:
> - Item_func_isempty(Item *a): Item_bool_func(a) {}
> - longlong val_int();
> + Item_geom_bool_func(Item *a): Item_bool_func(a) {}
> optimize_type select_optimize() const { return OPTIMIZE_NONE; }
> + void fix_length_and_dec() { set_char_length_and_dec(1);
maybe_null= 1; }
> +};
maybe_null was not set before. Is this a functional change?
...
> @@ -282,11 +294,8 @@ public:
> Item_func_x(Item *a): Item_real_func(a) {}
> double val_real();
> const char *func_name() const { return "x"; }
> - void fix_length_and_dec()
> - {
> - Item_real_func::fix_length_and_dec();
> - maybe_null= 1;
> - }
> + void fix_length_and_dec()
> + { Item_real_func::fix_length_and_dec(); maybe_null= 1; }
> };
>
Personally, I do not think this is a change to the better, but that's
a matter of personal taste.
...
> === modified file 'sql/item_strfunc.cc'
> --- a/sql/item_strfunc.cc 2009-01-09 18:43:23 +0000
> +++ b/sql/item_strfunc.cc 2009-03-04 06:59:35 +0000
> @@ -100,7 +100,28 @@ longlong Item_str_func::val_int()
> }
>
>
> -String *Item_func_md5::val_str(String *str)
> +String *Item_str_ascii_func::val_str(String *str)
> +{
> + DBUG_ASSERT(fixed == 1);
> +
> + if (!(collation.collation->state & MY_CS_NONASCII))
> + return val_ascii(str);
> +
> + uint errors;
> + String *res= val_ascii(&ascii_buf);
> + if (!res)
> + return 0;
> +
> + if ((null_value= str->copy(res->ptr(), res->length(),
> + &my_charset_latin1, collation.collation,
> + &errors)))
> + return 0;
> +
> + return str;
> +}
This is very identical to Item::val_ascii, only difference is that
val_ascii is called instead of val_str. This code duplication
indicates that there is something fishy with this design ...
...
> @@ -3307,11 +3332,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
> + uint dot_length= (p <= buf) ? 1 : 0;
> + (void) str->append(num + 4 - length, length - dot_length,
> + &my_charset_latin1);
> }
> - str->length(str->length()-1); // Remove last '.';
> return str;
> }
The new version seems a bit different from the previous one. Is there
a bug fix hidden here?
...
> === modified file 'sql/item_timefunc.h'
> --- a/sql/item_timefunc.h 2009-02-07 16:00:57 +0000
> +++ b/sql/item_timefunc.h 2009-03-04 06:59:35 +0000
> @@ -708,43 +619,7 @@ class Item_extract :public Item_int_func
> };
>
>
> -class Item_typecast :public Item_str_func
> -{
> -public:
> - Item_typecast(Item *a) :Item_str_func(a) {}
> - String *val_str(String *a)
> - {
> - DBUG_ASSERT(fixed == 1);
> - String *tmp=args[0]->val_str(a);
> - null_value=args[0]->null_value;
> - if (tmp)
> - tmp->set_charset(collation.collation);
> - return tmp;
> - }
> - void fix_length_and_dec()
> - {
> - collation.set(&my_charset_bin);
> - max_length=args[0]->max_length;
> - }
> - virtual const char* cast_type() const= 0;
> - virtual void print(String *str, enum_query_type query_type);
> -};
> -
> -
> -class Item_typecast_maybe_null :public Item_typecast
> -{
> -public:
> - Item_typecast_maybe_null(Item *a) :Item_typecast(a) {}
> - void fix_length_and_dec()
> - {
> - collation.set(&my_charset_bin);
> - max_length=args[0]->max_length;
> - maybe_null= 1;
> - }
> -};
> -
> -
> -class Item_char_typecast :public Item_typecast
> +class Item_char_typecast :public Item_str_func
> {
> int cast_length;
> CHARSET_INFO *cast_cs, *from_cs;
> @@ -752,23 +627,31 @@ class Item_char_typecast :public Item_ty
> String tmp_value;
> public:
> Item_char_typecast(Item *a, int length_arg, CHARSET_INFO *cs_arg)
> - :Item_typecast(a), cast_length(length_arg), cast_cs(cs_arg) {}
> + :Item_str_func(a), cast_length(length_arg), cast_cs(cs_arg) {}
> enum Functype functype() const { return CHAR_TYPECAST_FUNC; }
> bool eq(const Item *item, bool binary_cmp) const;
> const char *func_name() const { return "cast_as_char"; }
> - const char* cast_type() const { return "char"; };
> String *val_str(String *a);
> void fix_length_and_dec();
> virtual void print(String *str, enum_query_type query_type);
> };
>
>
> -class Item_date_typecast :public Item_typecast_maybe_null
> +class Item_typecast :public Item_str_ascii_func
> +{
> +public:
> + Item_typecast(Item *a) :Item_str_ascii_func(a) {}
> + virtual const char* cast_type() const= 0;
> + virtual void print(String *str, enum_query_type query_type);
> +};
> +
> +
> +class Item_date_typecast :public Item_typecast
Why have you changed the order of the class definitions here? Makes
the diff harder to read.
...
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc 2009-02-13 17:44:21 +0000
> +++ b/sql/sql_select.cc 2009-03-04 06:59:35 +0000
> @@ -13660,10 +13660,10 @@ static Field *create_tmp_field_from_item
> Field_long : make them Field_longlong.
> */
> if (item->max_length >= (MY_INT32_NUM_DECIMAL_DIGITS - 1))
> - new_field=new Field_longlong(item->max_length, maybe_null,
> + new_field=new Field_longlong(item->max_char_length(), maybe_null,
> item->name, item->unsigned_flag);
> else
> - new_field=new Field_long(item->max_length, maybe_null,
> + new_field=new Field_long(item->max_char_length(), maybe_null,
> item->name, item->unsigned_flag);
Why should max_char_length() be used in the constructors and not in the
if-test? And why is not the same change done for REAL_RESULT?
...
