Alexander Barkov wrote:
> Hi Øystein, Sergey,
>
> Øystein, Thank you very much for your review!
> I previously send bried reply. This letter contains a detailed reply.
Thanks, see my response inline.
...
>> > @@ -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?
>
> This is to pass correct input to str_to_datetime_with_warn().
>
> Previous code with my_charset_bin didn't work for real
> multi-byte character sets, because in case of UCS2
> str_to_date_time_with_warn() would get:
>
> '\0' '1' '\0' '9' '\0' '9' '\0' '9' '\0' '-' '\0' '0' '\0' '1'
>
> instead of
>
> '1999-01'. I.t. extra 0x00 bytes would be in the string.
>
>
> I use "latin1" here just because we don't have a special
> built-in character set "ascii".
> However, latin1 can perfectly work instead of ascii.
>
> Note, any built-in character set which encodes digits as single
> byte would work as well:
>
> '0' = 0x30
> '1' = 0x31
> ...
> '9' = 0x39
>
This explains why the change was needed. My question was why it
should be latin1 and not default_charset().
>>
>> ...
>>
>> > @@ -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.
>
>
> I use "decimal" also as a temporary variable here.
>
>
> With set_char_length_and_dec() it will look more cumbersome:
>
>
> str2my_decimal(E_DEC_FATAL_ERROR, str_arg, length, charset,
> &decimal_value);
> name= (char*) str_arg;
> fixed= 1;
>
set_char_length_and_dec(my_decimal_precision_to_length(decimal_value.intg +
> decimal_value.frac,
> decimal_value.frac,
> unsigned_flag),
> decimal_value.frac);
>
> 8 lines of code instead of 6.
>
> What do you think?
>
As I said in the first email, I think you should drop
set_char_length_and_dec entirely. So I do not suggest that you should
use 8 lines instead of 6. Rather, do it the same way in the single
case that use set_char_length_and_dec.
>
>
>>
>> ...
>>
>> > @@ -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.
>
>
> I think I will add a temporary variable, say char_length,
> and will do this instead:
>
> set_char_length(char_length);
>
> Do you agree?
I think that is a good idea.
...
>> ...
>>
>> > @@ -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)
>>
>
> You're right, a forward declaration of my_decimal2string() and
> type "my_decimal" in sql_string.h would do. In other hand,
> keeping things separately is a good thing.
> Not sure which is preferable though :)
>
I suggest to keep it the way it is now and avoid that String depends
on my_decimal.
>
>
>> > +*/
>> > +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?
>
> set_set_decimal() always use dot as a decimal point.
>
>
> Locale dependent decimal points and thousand separators are supported
> only in the function format():
>
> SELECT FORMAT(1234.5645656, 3, 'de_DE');
OK.
>
>
>>
>> ...
>>
>> > @@ -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?
>
> I think all lines of the code (except the last one) use "char length"
> notation.
>
> The last line converts into byte notation:
>
> set_char_length_and_dec(max_length);
>
>
> Perhaps with adding a temporary variable "char_length", similar to the
> above cases, would look less confusing.
>
> What do you think?
I guess the assumption here is that each decimal is always one
character. Then it should be correct.
Using a temporary variable "char_length" would make things a bit
clearer.
>
>
>>
>> >
>> >
>> > @@ -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.
>
> Item_func_round::fix_length_and_dec() has a number of "returns"
> in the middle of the body.
>
> Collation must be set before we calculate max_length.
>
> Perhaps, to make it clearer, we should add a temporary
> variable char_length again, replace all "return" to
> "goto end" and then put this code:
>
>
> end:
> collation.set(default_charset(), DERIVATION_COERCIBLE,
> MY_REPERTOIRE_ASCII);
> set_char_length(char_length);
> }
>
>
> What do you think?
When I wrote my comment, I did not realize that set_char_length had
the side effect of setting collation. Since this code for some reason
does not use set_char_lenght(), I now understand why this has to be
done first. By the way, it does not seem that you recall this side
effect either since you suggest to set it before calling
set_char_length. Which kind of emphasizes my point that this side
effect is confusing.
And setting it first is definitely better than using goto. However,
could not collation be set once and for all in the constructor?
>
>
>>
>> > 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.
>
> The proposal about temporary variable "char_length" should fix it.
> Do you agree?
>
If you mean followed by "set_char_length(char_length)", I agree.
>
>>
>> ...
>>
>>
>> > @@ -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?
>
> No needs. Reverted.
>
>>
>> By the way, why is not set_char_length needed here?
>
> Item_func_set_user_var just copies all attributes from
> the expression.
>
> So:
>
> collation.set(args[0]->collation.collation, DERIVATION_IMPLICIT);
> max_length= args[0]->max_length;
>
> is effectively the same to:
>
> collation.set(args[0]->collation.collation, DERIVATION_IMPLICIT);
> set_char_length(args[0]->max_char_length);
>
> The first versrion requires less CPU (no extra devision and extra
> multiplication).
>
Sounds reasonable.
By the way, the above code is not according to the MySQL code standard
for assignment.
...
>> >
>> >
>> > === 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?
>
> Thank for noticing this. This change is not needed. Removed.
OK.
>
>>
>> >
>> > @@ -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.
>
> Yes, charsets of this and args[0] can be different.
> CAST(xxx AS SIGNED) returns @@character_set_connection in string
> context, while xxx can be of any other character sets.
>
> For example, xxx can be a table column,
> or a string literal with character set introducer,
> and so on:
>
>
> mysql> set @@character_set_connection=ucs2;
> Query OK, 0 rows affected (0.00 sec)
>
> mysql> select charset(cast(_latin1'1' as signed));
> +-------------------------------------+
> | charset(cast(_latin1'1' as signed)) |
> +-------------------------------------+
> | ucs2 |
> +-------------------------------------+
> 1 row in set (0.00 sec)
>
>
> In this example:
>
> - charset of args[0] is latin1
> - charset of this is ucs2
>
OK. Thanks for explaining.
>
>
>>
>> > 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?
>
> max_length is initialized in constructor, in this line:
>
> max_length= my_decimal_precision_to_length(len, dec, unsigned_flag);
>
> and means "number of characters".
>
> charset is not know at this point yet.
This part I do not understand. set_char_length sets charset to
default_charset(), which I thought was the charset of the connection.
Is this not known when the item is constructed? Is this because
charset of connection could change between compilation and execution?
>
>
> When we do fix_length_and_dec(), charset is already known.
> So we just convert from "number of characters" to "number of bytes".
>
> It gives some inconsistency in what "max_length" means though
> in different points of time.
>
> What do you suggest?
>
I see your issues. If charset is not know when object is constructed, I
have no better suggestion.
>
>
>>
>> ...
>> > @@ -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?
>
> Yes, this is a bug.
> MySQL creates a minimal possible data type.
> sign() didn't follow this rule. In 5.0 you get:
>
> mysql> create table t1 as select concat(sign(-1)) as c;
> Query OK, 1 row affected (0.01 sec)
> Records: 1 Duplicates: 0 Warnings: 0
> mysql> explain t1;
> +-------+---------------+------+-----+---------+-------+
> | Field | Type | Null | Key | Default | Extra |
> +-------+---------------+------+-----+---------+-------+
> | c | varbinary(21) | NO | | | |
> +-------+---------------+------+-----+---------+-------+
> 1 row in set (0.04 sec)
>
>
> With WL#2649 it will work as follows:
>
>
> mysql> create table t1 as select concat(sign(-1)) as c;
> Query OK, 1 row affected (0.04 sec)
> Records: 1 Duplicates: 0 Warnings: 0
> mysql> explain t1;
> +-------+------------+------+-----+---------+-------+
> | Field | Type | Null | Key | Default | Extra |
> +-------+------------+------+-----+---------+-------+
> | c | varchar(2) | NO | | | |
> +-------+------------+------+-----+---------+-------+
> 1 row in set (0.01 sec)
>
Thanks for explaining. Do you plan to document which bugs are fixed
by this work log?
>
>
>>
>> ...
>>
>> > @@ -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?
>
> The same problem here. ascii() has been one of the rare functions
> which didn't create minimal possible data type.
>
>
OK.
>>
>> ...
>>
>> > @@ -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?
>
> Oops. This is a leftover from a previous patch version.
>
> Item_int_func::fix_length_and_dec() is empty now.
> There's no need to call it at all.
> I will remove all calls for Item_int_func::fix_length_and_dec().
>
> Do you agree?
I am a bit confused here. Item_int_func::fix_length_and_dec() was not
empty in the first patch that was committed. I see it is empty now.
However, your latest patch still contains calls to it.
>
>
>>
>> > 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?
>
> val_ascii() is used only in cases when ASCII input is expected.
>
> On other cases, @@character_set_connection will be used:
>
>
> mysql> set @@character_set_connection=ucs2; select
> charset(AsWKT(PointFromText('Point(1 1)')));
> Query OK, 0 rows affected (0.00 sec)
>
> +---------------------------------------------+
> | charset(AsWKT(PointFromText('Point(1 1)'))) |
> +---------------------------------------------+
> | ucs2 |
> +---------------------------------------------+
> 1 row in set (0.00 sec)
>
>
I do not think I understand how this works. As far as I can tell, if
one calls val_str on an Item_func_as_wkt object,
Item_str_ascii_func::val_str will be called, and that bases its result
on Item_func_as_wkt::val_ascii. How does the default character set
come into play?
>
>> And why do you not use set_char_length() to set the
>> collation and length?
>
> This is because we have the TEXT data type, not VARCHAR.
> TEXT is limited in *bytes*, not in characters.
>
>
OK.
...
>> > @@ -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?
>
> Diff doesn't look nice here.
>
> What happes here is:
>
> I added a new intermediate class Item_geom_bool_func,
> and incorporated all common features of these three classes:
>
> - Item_func_isempty
> - Item_func_issimple
> - Item_func_isclosed
>
> These classes all used to set maybe_null to 1
> in their respected fix_length_and_dec() methods.
>
> I just moved this code into this new parent class.
>
> No behaviour change.
>
OK.
>>
>> ...
>>
>> > @@ -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 ...
>
> You're absolutely right they are identical - both do conversion.
> However, they do absolutely different conversion:
>
> - From ASCII to collation.collation
> vs
> - From collation.collation to ASCII.
>
>
> If an Item is ASCII by its nature, then I define val_ascii() for it,
> and do conversion from ASCII to collation.collation in
> Item_str_ascii_func::val_str() when we need val_str().
>
> If an Item is not ASCII by its nature, then I define val_str(),
> and do conversion from @@character_set_connection to ASCII in
> Item::val_ascii() when we need val_ascii().
>
>
> I don't think anything which really simplifies the code
> can be done here.
>
> What do you suggest?
OK. Please, ignore my comment. I did not quite understand the
interactions between val_str and val_ascii when I wrote that.
...
Thanks for your patience with explaining things that I did not
understand,
--
Øystein
