List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:February 15 2011 12:32pm
Subject:Re: bzr commit into mysql-trunk branch (jorgen.loland:3578) Bug#59405
View as plain text  
Hi Jørgen,

Thanks for the patch.  Code changes looks good.  I have some minor 
suggestions.  I also suggest to include in the test case some scenarios 
where arguments are actually NULL.  See in-line for comments.

On 01/31/2011 02:39 PM, Jorgen Loland wrote:
> #At file:///export/home/jl208045/mysql/mysql-trunk-59405/ based on
> revid:sandeep.doddaballapur@stripped
>
>   3578 Jorgen Loland	2011-01-31
>        BUG#59405: FIND_IN_SET won't work normaly after upgrade
>                   from 5.1 to 5.5
>
>        In this bug, args[0] in an Item_func_find_in_set stored an
>        Item_func_weekday that was constant. In
>        Item_func_find_in_set::fix_length_and_dec(), args[0]->val_str()
>        was called. Later, when Item_func_find_in_set::val_int() was
>        called, args[0]->null_value was checked. However, the
>        Item_func_weekday in args[0] had now been replaced with an
>        Item_cache. No val_*() calls had been made to this Item_cache,
>        thus null_value was incorrectly 'true', resulting in missing
>        rows in the result set.
>
>        The implemented fix is to remember args[0]->null_value as it
>        was right after calling it's val_str() in fix_length_and_dec().
>        Note: this only applies if args[0] is constant.
>
>        An alternative fix would be to call args[0]->val_int() inside
>        Item_func_find_in_set::val_int(), but that would have to be
>        done for every record this const value is checked against.
>       @ mysql-test/r/func_set.result
>          Add test for BUG#59405
>       @ mysql-test/t/func_set.test
>          Add test for BUG#59405
>       @ sql/item_func.cc
>          Add variable Item_func_find_in_set::args_0_null_value
>       @ sql/item_func.h
>          Add variable Item_func_find_in_set::args_0_null_value
>
>      modified:
>        mysql-test/r/func_set.result
>        mysql-test/t/func_set.test
>        sql/item_func.cc
>        sql/item_func.h
> === modified file 'mysql-test/r/func_set.result'
> --- a/mysql-test/r/func_set.result	2009-06-16 14:36:15 +0000
> +++ b/mysql-test/r/func_set.result	2011-01-31 13:39:14 +0000
> @@ -159,3 +159,15 @@ SELECT CONVERT( a USING latin1 ) FROM t2
>   CONVERT( a USING latin1 )
>
>   DROP TABLE t1, t2;
> +#
> +# BUG#59405: FIND_IN_SET won't work normaly after upgrade from 5.1 to 5.5
> +#
> +CREATE TABLE t1(days set('1','2','3','4','5','6','7'));
> +INSERT INTO t1 VALUES('1,2,3,4,5,6,7'), ('1,2,3,4,5,6,7');
> +
> +SELECT * FROM t1 WHERE FIND_IN_SET(DAYOFWEEK(CURRENT_DATE()),days);
> +days
> +1,2,3,4,5,6,7
> +1,2,3,4,5,6,7
> +
> +DROP TABLE t1;
>
> === modified file 'mysql-test/t/func_set.test'
> --- a/mysql-test/t/func_set.test	2009-06-16 14:36:15 +0000
> +++ b/mysql-test/t/func_set.test	2011-01-31 13:39:14 +0000
> @@ -97,3 +97,16 @@ SELECT CONVERT( a USING latin1 ) FROM t1
>   SELECT CONVERT( a USING latin1 ) FROM t2;
>
>   DROP TABLE t1, t2;
> +
> +--echo #
> +--echo # BUG#59405: FIND_IN_SET won't work normaly after upgrade from 5.1 to 5.5
> +--echo #
> +
> +CREATE TABLE t1(days set('1','2','3','4','5','6','7'));
> +INSERT INTO t1 VALUES('1,2,3,4,5,6,7'), ('1,2,3,4,5,6,7');

I suggest also including a row where value is NULL.  E.g.,

INSERT INTO t1 VALUES('1,2,3,4,5,6,7'), NULL, ('1,2,3,4,5,6,7');

> +
> +--echo
> +SELECT * FROM t1 WHERE FIND_IN_SET(DAYOFWEEK(CURRENT_DATE()),days);

To test some aspects of NULL values in arguments, I suggest adding a few 
queries like the following:

SELECT * FROM t1 WHERE FIND_IN_SET(DAYOFWEEK(CURRENT_DATE()),days) IS 
UNKNOWN;

SELECT * FROM t1 WHERE FIND_IN_SET(DAYOFWEEK(CURRENT_DATE()),NULL) IS 
UNKNOWN;

SELECT * FROM t1 WHERE FIND_IN_SET(NULL,days) IS UNKNOWN;

SELECT * FROM t1 WHERE FIND_IN_SET(NULL,NULL) IS UNKNOWN;

It could not find any existing tests for FIND_IN_SET with NULL arguments.

> +
> +--echo
> +DROP TABLE t1;
>
> === modified file 'sql/item_func.cc'
> --- a/sql/item_func.cc	2011-01-14 15:36:19 +0000
> +++ b/sql/item_func.cc	2011-01-31 13:39:14 +0000
> @@ -3019,6 +3019,12 @@ void Item_func_find_in_set::fix_length_a
>       if (field->real_type() == MYSQL_TYPE_SET)
>       {
>         String *find=args[0]->val_str(&value);
> +      /*
> +        Because it is constant, args[0] may be replaced by an Item_cache.
> +        Store null_value here to avoid args[0]->val_int() before checking
> +        the null_value in Item_func_find_in_set::val_int().
> +       */
> +      args_0_null_value= args[0]->null_value;
>         if (find)
>         {
>   	enum_value= find_type(((Field_enum*) field)->typelib,find->ptr(),
> @@ -3039,8 +3045,12 @@ longlong Item_func_find_in_set::val_int(
>     DBUG_ASSERT(fixed == 1);
>     if (enum_value)
>     {
> +    // enum_value is set iff args[0]->const_item() in fix_length_and_dec().
> +    DBUG_ASSERT(args[0]->const_item());
> +
>       ulonglong tmp=(ulonglong) args[1]->val_int();
> -    if (!(null_value=args[1]->null_value || args[0]->null_value))
> +    null_value= args[1]->null_value;

The above assignments are not equivalent.  Previously, null_value was 
true if either of the arguments where null.  Now, it seems it is only 
affected by argument 1.  However, I have not been able to produce a test 
case where this seems to matter.  But to be sure, I suggest adding the 
follow before calling args[1]->val_int():

   if (null_value= args_0_null_value) return 0;

(There is no need to evaluate argument 1 if argument 0 is NULL.)

> +    if (!null_value&&  !args_0_null_value)
>       {
>         if (tmp&  enum_bit)
>   	return enum_value;
>
> === modified file 'sql/item_func.h'
> --- a/sql/item_func.h	2011-01-28 13:49:59 +0000
> +++ b/sql/item_func.h	2011-01-31 13:39:14 +0000
> @@ -1025,9 +1025,11 @@ class Item_func_find_in_set :public Item
>     String value,value2;
>     uint enum_value;
>     ulonglong enum_bit;
> +  bool args_0_null_value;  /* Cache args[0]->null_value iff args[0] is const */

It seems to me that "arg0_" is a more common prefix in the code than 
"args_0_", but I will not insist on anything.

>     DTCollation cmp_collation;
>   public:
> -  Item_func_find_in_set(Item *a,Item *b) :Item_int_func(a,b),enum_value(0) {}
> +  Item_func_find_in_set(Item *a,Item *b)
> +    : Item_int_func(a,b), enum_value(0), args_0_null_value(false) {}
>     longlong val_int();
>     const char *func_name() const { return "find_in_set"; }
>     void fix_length_and_dec();
>
>
>
>
>

--
Øystein


Thread
bzr commit into mysql-trunk branch (jorgen.loland:3578) Bug#59405Jorgen Loland31 Jan
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3578) Bug#59405Øystein Grøvlen15 Feb