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

Thanks for fixing this.  Patch is approved, but see some suggestions
below.

On 02/ 9/11 02:25 PM, Jorgen Loland wrote:
 > #At file:///export/home/jl208045/mysql/mysql-trunk-59793/ based on 
revid:dmitry.shulga@stripped
 >
 >   3625 Jorgen Loland	2011-02-09
 >        Bug#59793: crash in Item_field::register_field_in_read_map
 >                   with view
 >
 >        Prior to the refactoring in this patch, Item_cond_xor behaved
 >        partially as an Item_cond and partially as an Item_func. The
 >        reasoning behind this was that XOR is currently not optimized
 >        (thus should be Item_func instead of Item_cond), but it was
 >        planned optimize it in the future (thus, made Item_cond anyway
 >        to ease optimization later).
 >
 >        Even though Item_cond inherits from Item_func, there are
 >        differences between these two. One difference is that the
 >        arguments are stored differently. Item_cond stores them in a
 >        list while Item_func store them in an args[].
 >
 >        BUG no 45221 was caused by Item_cond_xor storing arguments in
 >        the list while users of the objects would look for them in
 >        args[]. The fix back then was to store the arguments in both
 >        locations.
 >
 >        In this bug, Item_cond_xor initially gets two Item_field
 >        arguments. These are stored in the list inherited from
 >        Item_cond and in args[] inherited from Item_func. During
 >        resolution, find_field_in_view() replaces the Item_fields
 >        stored in the list with Item_direct_view_refs, but args[]
 >        still points to the unresolved Item_fields. This shows that
 >        the fix for 45221 was incorrect.
 >
 >        The refactoring performed in this patch removes the confusion
 >        by making the XOR item an Item_func period.
 >       @ mysql-test/include/subquery.inc
 >          Add test for BUG#59793
 >       @ mysql-test/r/negation_elimination.result
 >          Add tests for negation of XOR
 >       @ mysql-test/r/subquery_nomat_nosj.result
 >          Add test for BUG#59793
 >       @ mysql-test/r/subquery_none.result
 >          Add test for BUG#59793
 >       @ mysql-test/t/negation_elimination.test
 >          Add tests for negation of XOR
 >       @ sql/item_cmpfunc.cc
 >          Refactor XOR item: it is now a pure Item_func, inheriting 
from Item_bool_func2 instead of Item_cond
 >       @ sql/item_cmpfunc.h
 >          Refactor XOR item: it is now a pure Item_func, inheriting 
from Item_bool_func2 instead of Item_cond
 >       @ sql/item_func.h
 >          Refactor XOR item: it is now a pure Item_func, inheriting 
from Item_bool_func2 instead of Item_cond
 >       @ sql/sql_yacc.yy
 >          Refactor XOR item: it is now a pure Item_func, inheriting 
from Item_bool_func2 instead of Item_cond
 >       @ unittest/gunit/item-t.cc
 >          Add unit test for Item_func_xor
 >
 >      modified:
 >        mysql-test/include/subquery.inc
 >        mysql-test/r/negation_elimination.result
 >        mysql-test/r/subquery_nomat_nosj.result
 >        mysql-test/r/subquery_none.result
 >        mysql-test/t/negation_elimination.test
 >        sql/item_cmpfunc.cc
 >        sql/item_cmpfunc.h
 >        sql/item_func.h
 >        sql/sql_yacc.yy
 >        unittest/gunit/item-t.cc
 > === modified file 'mysql-test/include/subquery.inc'
 > --- a/mysql-test/include/subquery.inc	2011-02-02 09:04:55 +0000
 > +++ b/mysql-test/include/subquery.inc	2011-02-09 13:25:55 +0000
 > @@ -5286,3 +5286,19 @@ CREATE TABLE t(a VARCHAR(245) DEFAULT
 >   INSERT INTO t VALUES 
(''),(''),(''),(''),(''),(''),(''),(''),(''),(''),('');
 >   SELECT * FROM (SELECT default(a) FROM t GROUP BY a) d;
 >   DROP TABLE t;
 > +
 > +--echo #
 > +--echo # Bug#59793: crash in Item_field::register_field_in_read_map 
with view
 > +--echo #
 > +
 > +CREATE TABLE t1(a INT);
 > +CREATE VIEW v1 AS SELECT a FROM t1;
 > +
 > +INSERT INTO t1 VALUES (0),(1),(2);
 > +
 > +SELECT a FROM t1 WHERE a IN
 > + (SELECT a XOR a FROM v1)
 > +ORDER BY a;

Since this query is applicable to semijoin optimization (I have
checked on the backporting branch), I suggest adding this test case to
subquery_sj.inc instead.

...

 > === modified file 'mysql-test/t/negation_elimination.test'
 > --- a/mysql-test/t/negation_elimination.test	2005-07-28 00:22:47 +0000
 > +++ b/mysql-test/t/negation_elimination.test	2011-02-09 13:25:55 +0000
 > @@ -65,10 +65,40 @@ select * from t1 where not((a<  5 and a
 >   explain select * from t1 where ((a between 5 and 15) and (not(a 
like 10)));
 >   select * from t1 where ((a between 5 and 15) and (not(a like 10)));
 >
 > +--echo
 > +--echo # XOR (Note: XOR is negated by negating one of the operands)
 > +
 > +--echo # Should return 6,7
 > +SELECT * FROM t1 WHERE ((a > 5) XOR (a > 7));
 > +
 > +--echo # Should return 0..5,8..19
 > +SELECT * FROM t1 WHERE ((NOT (a > 5)) XOR (a > 7));
 > +SELECT * FROM t1 WHERE ((a > 5) XOR (NOT (a > 7)));
 > +SELECT * FROM t1 WHERE NOT ((a > 5) XOR (a > 7));
 > +
 > +--echo # Should return 6,7
 > +SELECT * FROM t1 WHERE NOT ((NOT (a > 5)) XOR (a > 7));
 > +SELECT * FROM t1 WHERE NOT ((a > 5) XOR (NOT (a > 7)));
 > +
 > +--echo # Should return 0..5,8..19
 > +SELECT * FROM t1 WHERE NOT (NOT (a > 5) XOR (NOT (a > 7)));

I suggest "NOT ((NOT (a > 5)) XOR (NOT (a > 7)))" which follows the 
pattern used in the other queries.

 > +
 > +SELECT * FROM t1 WHERE (NULL XOR (a > 7));
 > +SELECT * FROM t1 WHERE NOT (NULL XOR (a > 7));
 > +
 > +--echo # Should be simplified to "...WHERE (a XOR a)
 > +EXPLAIN EXTENDED SELECT * FROM t1 WHERE NOT ((NOT a) XOR (a));
 > +
 > +--echo # Should be simplified to "...WHERE (a XOR a)
 > +EXPLAIN EXTENDED SELECT * FROM t1 WHERE NOT (a XOR (NOT a));
 > +
 > +--echo # XOR End
 > +--echo
 > +
 >   delete from t1 where a>  3;
 >   select a, not(not(a)) from t1;
 >   explain extended select a, not(not(a)), not(a<= 2 and not(a)), 
not(a not like "1"), not (a not in (1,2)), not(a != 2) from t1 where 
not(not(a)) having not(not(a));
 >
 >   drop table t1;
 >
 > -# End of 4.1 tests
 > +
 >
 > === modified file 'sql/item_cmpfunc.cc'
 > --- a/sql/item_cmpfunc.cc	2011-02-08 15:54:12 +0000
 > +++ b/sql/item_cmpfunc.cc	2011-02-09 13:25:55 +0000
 > @@ -5333,17 +5333,15 @@ bool Item_func_like::turboBM_matches(con
 >       very fast to use.
 >   */
 >
 > -longlong Item_cond_xor::val_int()
 > +longlong Item_func_xor::val_int()
 >   {
 >     DBUG_ASSERT(fixed == 1);
 > -  List_iterator<Item>  li(list);
 > -  Item *item;
 > -  int result=0;	
 > +  int result=0;
 >     null_value=0;
 > -  while ((item=li++))
 > +  for (uint i= 0; i<  arg_count; i++)
 >     {
 > -    result^= (item->val_int() != 0);
 > -    if (item->null_value)
 > +    result^= (args[i]->val_int() != 0);
 > +    if (args[i]->null_value)
 >       {
 >         null_value=1;
 >         return 0;
 > @@ -5390,6 +5388,33 @@ Item *Item_bool_rowready_func2::neg_tran
 >     return item;
 >   }
 >
 > +/**
 > +  XOR can be negated by negating one of the operands:
 > +
 > +  NOT (a XOR b)  =>  (NOT a) XOR b
 > +                 =>  a       XOR (NOT b)
 > +
 > +  @param thd     Thread handle
 > +  @return        New negated item
 > +*/
 > +Item *Item_func_xor::neg_transformer(THD *thd)
 > +{
 > +  Item *neg_operand;
 > +  Item_func_xor *new_item;
 > +  if ((neg_operand= args[0]->neg_transformer(thd)))
 > +    // args[0] has neg_tranformer
 > +    new_item= new(thd->mem_root) Item_func_xor(neg_operand, args[1]);
 > +  else if ((neg_operand= args[1]->neg_transformer(thd)))
 > +    // args[1] has neg_tranformer
 > +    new_item= new(thd->mem_root) Item_func_xor(args[0], neg_operand);
 > +  else
 > +  {
 > +    neg_operand= new(thd->mem_root) Item_func_not(args[0]);
 > +    new_item= new(thd->mem_root) Item_func_xor(neg_operand, args[1]);
 > +  }
 > +  return new_item;
 > +}
 > +


I think you should add something in the commit comments about having
added a neg_transformer and why that was necessary.


 >
 >   /**
 >     a IS NULL  ->   a IS NOT NULL.
 >
 > === modified file 'sql/item_cmpfunc.h'
 > --- a/sql/item_cmpfunc.h	2010-12-29 00:38:59 +0000
 > +++ b/sql/item_cmpfunc.h	2011-02-09 13:25:55 +0000
 > @@ -1,7 +1,7 @@
 >   #ifndef ITEM_CMPFUNC_INCLUDED
 >   #define ITEM_CMPFUNC_INCLUDED
 >
 > -/* Copyright (c) 2000, 2010, Oracle and/or its affiliates. All 
rights reserved.
 > +/* Copyright (c) 2000, 2011, Oracle and/or its affiliates. All 
rights reserved.
 >
 >      This program is free software; you can redistribute it and/or modify
 >      it under the terms of the GNU General Public License as published by
 > @@ -411,6 +411,22 @@ public:
 >     bool subst_argument_checker(uchar **arg) { return TRUE; }
 >   };
 >
 > +/**
 > +  XOR inherits from Item_bool_func2 because it is not optimized yet.
 > +  Later, when XOR is optimized, it needs to inherit from
 > +  Item_cond instead. See WL#5800.
 > +*/
 > +class Item_func_xor :public Item_bool_func2
 > +{
 > +public:
 > +  Item_func_xor(Item *i1, Item *i2) :Item_bool_func2(i1, i2) {}
 > +  enum Functype functype() const { return XOR_FUNC; }
 > +  const char *func_name() const { return "xor"; }
 > +  longlong val_int();
 > +  void top_level_item() {}
 > +  Item *neg_transformer(THD *thd);
 > +};
 > +
 >   class Item_func_not :public Item_bool_func
 >   {
 >   public:
 > @@ -1759,45 +1775,6 @@ inline bool is_cond_or(Item *item)
 >     return (cond_item->functype() == Item_func::COND_OR_FUNC);
 >   }
 >
 > -/*
 > -  XOR is Item_cond, not an Item_int_func because we could like to
 > -  optimize (a XOR b) later on. It's low prio, though
 > -*/
 > -
 > -class Item_cond_xor :public Item_cond
 > -{
 > -public:
 > -  Item_cond_xor(Item *i1,Item *i2) :Item_cond(i1,i2)
 > -  {
 > -    /*
 > -      Items must be stored in args[] as well because this Item_cond is
 > -      treated as a FUNC_ITEM (see type()). I.e., users of it will get
 > -      it's children by calling arguments(), not argument_list(). This
 > -      is a temporary solution until XOR is optimized and treated like
 > -      a full Item_cond citizen.
 > -     */
 > -    arg_count= 2;
 > -    args= tmp_arg;
 > -    args[0]= i1;
 > -    args[1]= i2;
 > -  }
 > -  enum Functype functype() const { return COND_XOR_FUNC; }
 > -  /* TODO: remove the next line when implementing XOR optimization */
 > -  enum Type type() const { return FUNC_ITEM; }
 > -  longlong val_int();
 > -  const char *func_name() const { return "xor"; }
 > -  void top_level_item() {}
 > -  /* Since child Items are stored in args[], Items cannot be added.
 > -     However, since Item_cond_xor is treated as a FUNC_ITEM (see
 > -     type()), the methods below should never be called.
 > -  */
 > -  bool add(Item *item) { DBUG_ASSERT(FALSE); return FALSE; }
 > -  bool add_at_head(Item *item) { DBUG_ASSERT(FALSE); return FALSE; }
 > -  bool add_at_head(List<Item>  *nlist) { DBUG_ASSERT(FALSE); return 
FALSE; }
 > -  void copy_andor_arguments(THD *thd, Item_cond *item) { 
DBUG_ASSERT(FALSE); }
 > -};
 > -
 > -
 >   /* Some useful inline functions */
 >
 >   inline Item *and_conds(Item *a, Item *b)
 >
 > === modified file 'sql/item_func.h'
 > --- a/sql/item_func.h	2011-02-08 15:54:12 +0000
 > +++ b/sql/item_func.h	2011-02-09 13:25:55 +0000
 > @@ -46,7 +46,7 @@ public:
 >     enum Functype { 
UNKNOWN_FUNC,EQ_FUNC,EQUAL_FUNC,NE_FUNC,LT_FUNC,LE_FUNC,
 >   		  GE_FUNC,GT_FUNC,FT_FUNC,
 >   		  LIKE_FUNC,ISNULL_FUNC,ISNOTNULL_FUNC,
 > -		  COND_AND_FUNC, COND_OR_FUNC, COND_XOR_FUNC,
 > +		  COND_AND_FUNC, COND_OR_FUNC, XOR_FUNC,
 >                     BETWEEN, IN_FUNC, MULT_EQUAL_FUNC,
 >   		  INTERVAL_FUNC, ISNOTNULLTEST_FUNC,
 >   		  SP_EQUALS_FUNC, SP_DISJOINT_FUNC,SP_INTERSECTS_FUNC,
 >
 > === modified file 'sql/sql_yacc.yy'
 > --- a/sql/sql_yacc.yy	2011-02-03 10:13:06 +0000
 > +++ b/sql/sql_yacc.yy	2011-02-09 13:25:55 +0000
 > @@ -7620,7 +7620,7 @@ expr:
 >           | expr XOR expr %prec XOR
 >             {
 >               /* XOR is a proprietary extension */
 > -            $$ = new (YYTHD->mem_root) Item_cond_xor($1, $3);
 > +            $$ = new (YYTHD->mem_root) Item_func_xor($1, $3);
 >               if ($$ == NULL)
 >                 MYSQL_YYABORT;
 >             }
 >
 > === modified file 'unittest/gunit/item-t.cc'
 > --- a/unittest/gunit/item-t.cc	2011-02-07 13:03:47 +0000
 > +++ b/unittest/gunit/item-t.cc	2011-02-09 13:25:55 +0000
 > @@ -183,4 +183,40 @@ TEST_F(ItemTest, ItemFuncDesDecrypt)
 >     EXPECT_LE(item_decrypt->max_length, length);
 >   }
 >
 > +
 > +TEST_F(ItemTest, ItemFuncXor)
 > +{
 > +  const uint length= 1U;
 > +  Item_int *item_one= new Item_int(0, length);
 > +  Item_int *item_two= new Item_int(1, length);
 > +
 > +  Item_func_xor *item_xor=
 > +    new Item_func_xor(item_one, item_two);
 > +
 > +  EXPECT_FALSE(item_xor->fix_fields(m_thd, NULL));
 > +  EXPECT_EQ(1, item_xor->val_int());
 > +
 > +  String print_buffer;
 > +  item_xor->print(&print_buffer, QT_ORDINARY);
 > +  EXPECT_STREQ("(0 xor 1)", print_buffer.c_ptr());
 > +
 > +  Item *neg_xor= item_xor->neg_transformer(m_thd);
 > +  EXPECT_FALSE(neg_xor->fix_fields(m_thd, NULL));
 > +  EXPECT_EQ(0, neg_xor->val_int());
 > +  EXPECT_DOUBLE_EQ(0.0, neg_xor->val_real());
 > +  EXPECT_FALSE(neg_xor->val_bool());
 > +  EXPECT_EQ(0, neg_xor->is_null());
 > +
 > +  print_buffer= String();
 > +  neg_xor->print(&print_buffer, QT_ORDINARY);
 > +  EXPECT_STREQ("((not(0)) xor 1)", print_buffer.c_ptr());
 > +
 > +  Item_func_xor *item_xor_null=
 > +    new Item_func_xor(item_one, new Item_null());
 > +  EXPECT_FALSE(item_xor_null->fix_fields(m_thd, NULL));
 > +
 > +  EXPECT_EQ(0, item_xor_null->val_int());
 > +  EXPECT_TRUE(item_xor_null->is_null());
 > +}

Nice unit test. If I where to suggest some extensions, it would be to
also test the result of two items with equal value.  Another extension
could be to verify the output of some of the functions inherited from
super classes (e.g., Item_bool_func2).

-- 
Øystein
Thread
bzr commit into mysql-trunk branch (jorgen.loland:3625) Bug#59793Jorgen Loland9 Feb
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3625) Bug#59793Tor Didriksen10 Feb
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3625) Bug#59793Øystein Grøvlen17 Feb