Bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.

Currently in the ONLY_FULL_GROUP_BY mode no hidden fields are allowed in the
select list. To ensure this each expression in the select list is checked
to be a constant, an aggregate function or to occur in the GROUP BY list.
The last two requirements are wrong and doesn't allow valid expressions like
"MAX(b) - MIN(b)" or "a + 1" in a query with grouping by a.

The correct check implemented by the patch will ensure that:
any field reference in the [sub]expressions of the select list 
  is under an aggregate function or
  is mentioned as member of the group list or
  is an outer reference or
  is part of the select list element that coincide with a grouping element.

The Item_field objects now can contain the position of the select list
expression which they belong to. The position is saved during the
field's Item_field::fix_fields() call.

The non_agg_fields list for non-aggregated fields is added to the SELECT_LEX
class. The SELECT_LEX::cur_pos_in_select_list now contains the position in the
select list of the expression being currently fixed.


sql/item.cc:
  Bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.
  The Item_field objects now contain the position of the select list
  expression which they belong to. The position is saved at the field's
  Item_field::fix_fields() call.
sql/item.h:
  Bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.
  The Item_field objects now can store the position in the select list of the
  expression to which they are belongs to.
sql/mysql_priv.h:
  Bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.
  Added the UNDEF_POS constant.
sql/sql_base.cc:
  Bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.
  Now the setup_fields() function maintains the cur_pos_in_select_list variable.
sql/sql_lex.cc:
  Bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.
  Set the cur_pos_in_select_list variable and the non_agg_fields list to their initial state.
sql/sql_lex.h:
  Bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.
  The non_agg_fields list for non-aggregated fields is added to the SELECT_LEX
  class. The SELECT_LEX::cur_pos_in_select_list now stores the position in the
  select list of the expression being currently fixed.
sql/sql_select.cc:
  Bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.
  Each select now keeps the list of fields that aren't
  used under any aggregate function. If an expression from the select list
  isn't found in the GROUP BY list the setup_group() function additionally
  checks whether non-aggregated fields occur in that expression.
  If there at least one such field and it isn't found in the GROUP BY list
  then an error is thrown.
sql/sql_union.cc:
  Bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.
  Clean up of the non_agg_fields list.
mysql-test/r/group_by.result:
  Added a test case for the bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.
mysql-test/t/group_by.test:
  Added a test case for the bug#23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode.

mysql-test/r/group_by.result

@@ -933,3 +933,105 @@ b	sum(1)
 18	6
 19	6
 DROP TABLE t1;
+CREATE TABLE t1 (a INT PRIMARY KEY, b INT);
+INSERT INTO t1 VALUES (1,1),(2,1),(3,2),(4,2),(5,3),(6,3);
+SET SQL_MODE = 'ONLY_FULL_GROUP_BY';
+SELECT MAX(a)-MIN(a) FROM t1 GROUP BY b;
+MAX(a)-MIN(a)
+1
+1
+1
+SELECT CEILING(MIN(a)) FROM t1 GROUP BY b;
+CEILING(MIN(a))
+1
+3
+5
+SELECT CASE WHEN AVG(a)>=0 THEN 'Positive' ELSE 'Negative' END FROM t1 
+GROUP BY b;
+CASE WHEN AVG(a)>=0 THEN 'Positive' ELSE 'Negative' END
+Positive
+Positive
+Positive
+SELECT a + 1 FROM t1 GROUP BY a;
+a + 1
+2
+3
+4
+5
+6
+7
+SELECT a + b FROM t1 GROUP BY b;
+ERROR 42000: 'test.t1.a' isn't in GROUP BY
+SELECT (SELECT t1_outer.a FROM t1 AS t1_inner GROUP BY b LIMIT 1) 
+FROM t1 AS t1_outer;
+(SELECT t1_outer.a FROM t1 AS t1_inner GROUP BY b LIMIT 1)
+1
+2
+3
+4
+5
+6
+SELECT 1 FROM t1 as t1_outer GROUP BY a 
+HAVING (SELECT t1_outer.a FROM t1 AS t1_inner GROUP BY b LIMIT 1);
+1
+1
+1
+1
+1
+1
+1
+SELECT (SELECT t1_outer.a FROM t1 AS t1_inner LIMIT 1) 
+FROM t1 AS t1_outer GROUP BY t1_outer.b;
+ERROR 42000: 'test.t1_outer.a' isn't in GROUP BY
+SELECT 1 FROM t1 as t1_outer GROUP BY a 
+HAVING (SELECT t1_outer.b FROM t1 AS t1_inner LIMIT 1);
+ERROR 42S22: Unknown column 'test.t1_outer.b' in 'field list'
+SELECT (SELECT SUM(t1_inner.a) FROM t1 AS t1_inner LIMIT 1) 
+FROM t1 AS t1_outer GROUP BY t1_outer.b;
+(SELECT SUM(t1_inner.a) FROM t1 AS t1_inner LIMIT 1)
+21
+21
+21
+SELECT (SELECT SUM(t1_inner.a) FROM t1 AS t1_inner GROUP BY t1_inner.b LIMIT 1)
+FROM t1 AS t1_outer;
+(SELECT SUM(t1_inner.a) FROM t1 AS t1_inner GROUP BY t1_inner.b LIMIT 1)
+3
+3
+3
+3
+3
+3
+SELECT (SELECT SUM(t1_outer.a) FROM t1 AS t1_inner LIMIT 1) 
+FROM t1 AS t1_outer GROUP BY t1_outer.b;
+ERROR 42000: 'test.t1_outer.a' isn't in GROUP BY
+SELECT 1 FROM t1 as t1_outer 
+WHERE (SELECT t1_outer.b FROM t1 AS t1_inner GROUP BY t1_inner.b LIMIT 1);
+1
+1
+1
+1
+1
+1
+1
+SELECT b FROM t1 GROUP BY b HAVING CEILING(b) > 0;
+b
+1
+2
+3
+SELECT 1 FROM t1 GROUP BY b HAVING b = 2 OR b = 3 OR SUM(a) > 12;
+1
+1
+1
+SELECT 1 FROM t1 GROUP BY b HAVING ROW (b,b) = ROW (1,1);
+1
+1
+SELECT 1 FROM t1 GROUP BY b HAVING a = 2;
+ERROR 42S22: Unknown column 'a' in 'having clause'
+SELECT 1 FROM t1 GROUP BY SUM(b);
+ERROR HY000: Invalid use of group function
+SELECT b FROM t1 AS t1_outer GROUP BY a HAVING t1_outer.a IN 
+(SELECT SUM(t1_inner.b)+t1_outer.b FROM t1 AS t1_inner GROUP BY t1_inner.a
+HAVING SUM(t1_inner.b)+t1_outer.b > 5);
+ERROR 42000: 'test.t1_outer.b' isn't in GROUP BY
+DROP TABLE t1;
+SET SQL_MODE = '';

mysql-test/t/group_by.test

@@ -701,3 +701,54 @@ EXPLAIN SELECT SQL_BIG_RESULT b, sum(1) FROM t1 GROUP BY b;
 SELECT b, sum(1) FROM t1 GROUP BY b;
 SELECT SQL_BIG_RESULT b, sum(1) FROM t1 GROUP BY b;
 DROP TABLE t1;
+
+#
+# Bug #23417: Too strict checks against GROUP BY in the ONLY_FULL_GROUP_BY mode
+#
+CREATE TABLE t1 (a INT PRIMARY KEY, b INT);
+INSERT INTO t1 VALUES (1,1),(2,1),(3,2),(4,2),(5,3),(6,3);
+
+SET SQL_MODE = 'ONLY_FULL_GROUP_BY';
+SELECT MAX(a)-MIN(a) FROM t1 GROUP BY b;
+SELECT CEILING(MIN(a)) FROM t1 GROUP BY b;
+SELECT CASE WHEN AVG(a)>=0 THEN 'Positive' ELSE 'Negative' END FROM t1 
+ GROUP BY b;
+SELECT a + 1 FROM t1 GROUP BY a;
+--error ER_WRONG_FIELD_WITH_GROUP 
+SELECT a + b FROM t1 GROUP BY b;
+SELECT (SELECT t1_outer.a FROM t1 AS t1_inner GROUP BY b LIMIT 1) 
+  FROM t1 AS t1_outer;
+SELECT 1 FROM t1 as t1_outer GROUP BY a 
+  HAVING (SELECT t1_outer.a FROM t1 AS t1_inner GROUP BY b LIMIT 1);
+--error ER_WRONG_FIELD_WITH_GROUP 
+SELECT (SELECT t1_outer.a FROM t1 AS t1_inner LIMIT 1) 
+  FROM t1 AS t1_outer GROUP BY t1_outer.b;
+--error ER_BAD_FIELD_ERROR 
+SELECT 1 FROM t1 as t1_outer GROUP BY a 
+  HAVING (SELECT t1_outer.b FROM t1 AS t1_inner LIMIT 1);
+SELECT (SELECT SUM(t1_inner.a) FROM t1 AS t1_inner LIMIT 1) 
+  FROM t1 AS t1_outer GROUP BY t1_outer.b;
+SELECT (SELECT SUM(t1_inner.a) FROM t1 AS t1_inner GROUP BY t1_inner.b LIMIT 1)
+  FROM t1 AS t1_outer;
+--error ER_WRONG_FIELD_WITH_GROUP 
+SELECT (SELECT SUM(t1_outer.a) FROM t1 AS t1_inner LIMIT 1) 
+  FROM t1 AS t1_outer GROUP BY t1_outer.b;
+
+SELECT 1 FROM t1 as t1_outer 
+  WHERE (SELECT t1_outer.b FROM t1 AS t1_inner GROUP BY t1_inner.b LIMIT 1);
+
+SELECT b FROM t1 GROUP BY b HAVING CEILING(b) > 0;
+
+SELECT 1 FROM t1 GROUP BY b HAVING b = 2 OR b = 3 OR SUM(a) > 12;
+SELECT 1 FROM t1 GROUP BY b HAVING ROW (b,b) = ROW (1,1);
+
+--error ER_BAD_FIELD_ERROR
+SELECT 1 FROM t1 GROUP BY b HAVING a = 2;
+--error ER_INVALID_GROUP_FUNC_USE
+SELECT 1 FROM t1 GROUP BY SUM(b);
+--error ER_WRONG_FIELD_WITH_GROUP 
+SELECT b FROM t1 AS t1_outer GROUP BY a HAVING t1_outer.a IN 
+  (SELECT SUM(t1_inner.b)+t1_outer.b FROM t1 AS t1_inner GROUP BY t1_inner.a
+   HAVING SUM(t1_inner.b)+t1_outer.b > 5);
+DROP TABLE t1;
+SET SQL_MODE = '';

sql/item.cc

@@ -3469,6 +3469,16 @@ Item_field::fix_outer_field(THD *thd, Field **from_field, Item **reference)
     {
       if (*from_field)
       {
+        if (thd->variables.sql_mode & MODE_ONLY_FULL_GROUP_BY &&
+            select->cur_pos_in_select_list != UNDEF_POS)
+        {
+          /*
+            As this is an outer field it should be added to the list of
+            non aggregated fields of the outer select.
+          */
+          marker= select->cur_pos_in_select_list;
+          select->non_agg_fields.push_back(this);
+        }
         if (*from_field != view_ref_found)
         {
           prev_subselect_item->used_tables_cache|= (*from_field)->table->map;
@@ -3671,10 +3681,11 @@ Item_field::fix_outer_field(THD *thd, Field **from_field, Item **reference)
 bool Item_field::fix_fields(THD *thd, Item **reference)
 {
   DBUG_ASSERT(fixed == 0);
+  Field *from_field= (Field *)not_found_field;
+  bool outer_fixed= false;
+
   if (!field)					// If field is not checked
   {
-    Field *from_field= (Field *)not_found_field;
-    bool outer_fixed= false;
     /*
       In case of view, find_field_in_tables() write pointer to view field
       expression to 'reference', i.e. it substitute that expression instead
@@ -3766,6 +3777,7 @@ bool Item_field::fix_fields(THD *thd, Item **reference)
         goto error;
       else if (!ret)
         return FALSE;
+      outer_fixed= 1;
     }
 
     set_field(from_field);
@@ -3809,6 +3821,13 @@ bool Item_field::fix_fields(THD *thd, Item **reference)
   }
 #endif
   fixed= 1;
+  if (thd->variables.sql_mode & MODE_ONLY_FULL_GROUP_BY &&
+      !outer_fixed && !thd->lex->in_sum_func &&
+      thd->lex->current_select->cur_pos_in_select_list != UNDEF_POS)
+  {
+    thd->lex->current_select->non_agg_fields.push_back(this);
+    marker= thd->lex->current_select->cur_pos_in_select_list;
+  }
   return FALSE;
 
 error:

sql/item.h

@@ -452,7 +452,8 @@ class Item {
   Item *next;
   uint32 max_length;
   uint name_length;                     /* Length of name */
-  uint8 marker, decimals;
+  int8 marker;
+  uint8 decimals;
   my_bool maybe_null;			/* If item may be null */
   my_bool null_value;			/* if item is null */
   my_bool unsigned_flag;

sql/mysql_priv.h

@@ -410,7 +410,8 @@ MY_LOCALE *my_locale_by_name(const char *name);
 #define UNCACHEABLE_EXPLAIN     8
 /* Don't evaluate subqueries in prepare even if they're not correlated */
 #define UNCACHEABLE_PREPARE    16
-
+/* Used to chack GROUP BY list in the MODE_ONLY_FULL_GROUP_BY mode */
+#define UNDEF_POS (-1)
 #ifdef EXTRA_DEBUG
 /*
   Sync points allow us to force the server to reach a certain line of code

sql/sql_base.cc

@@ -4393,6 +4393,7 @@ bool setup_fields(THD *thd, Item **ref_pointer_array,
     bzero(ref_pointer_array, sizeof(Item *) * fields.elements);
 
   Item **ref= ref_pointer_array;
+  thd->lex->current_select->cur_pos_in_select_list= 0;
   while ((item= it++))
   {
     if (!item->fixed && item->fix_fields(thd, it.ref()) ||
@@ -4408,7 +4409,10 @@ bool setup_fields(THD *thd, Item **ref_pointer_array,
 	sum_func_list)
       item->split_sum_func(thd, ref_pointer_array, *sum_func_list);
     thd->used_tables|= item->used_tables();
+    thd->lex->current_select->cur_pos_in_select_list++;
   }
+  thd->lex->current_select->cur_pos_in_select_list= UNDEF_POS;
+
   thd->lex->allow_sum_func= save_allow_sum_func;
   thd->set_query_id= save_set_query_id;
   DBUG_RETURN(test(thd->net.report_error));

sql/sql_lex.cc

@@ -1180,6 +1180,8 @@ void st_select_lex::init_select()
   offset_limit= 0;      /* denotes the default offset = 0 */
   with_sum_func= 0;
   is_correlated= 0;
+  cur_pos_in_select_list= UNDEF_POS;
+  non_agg_fields.empty();
 }
 
 /*

sql/sql_lex.h

@@ -581,6 +581,10 @@ class st_select_lex: public st_select_lex_node
   bool no_wrap_view_item;
   /* exclude this select from check of unique_table() */
   bool exclude_from_table_unique_test;
+  /* List of fields that aren't under an aggregate function */
+  List<Item_field> non_agg_fields;
+  /* index in the select list of the expression currently being fixed */
+  int cur_pos_in_select_list;
 
   List<udf_func>     udf_list;                  /* udf function calls stack */
 

sql/sql_select.cc

@@ -13214,49 +13214,83 @@ setup_group(THD *thd, Item **ref_pointer_array, TABLE_LIST *tables,
 	    bool *hidden_group_fields)
 {
   *hidden_group_fields=0;
+  ORDER *ord;
+
   if (!order)
     return 0;				/* Everything is ok */
 
-  if (thd->variables.sql_mode & MODE_ONLY_FULL_GROUP_BY)
-  {
-    Item *item;
-    List_iterator<Item> li(fields);
-    while ((item=li++))
-      item->marker=0;			/* Marker that field is not used */
-  }
   uint org_fields=all_fields.elements;
 
   thd->where="group statement";
-  for (; order; order=order->next)
+  for (ord= order; ord; ord= ord->next)
   {
-    if (find_order_in_list(thd, ref_pointer_array, tables, order, fields,
+    if (find_order_in_list(thd, ref_pointer_array, tables, ord, fields,
 			   all_fields, TRUE))
       return 1;
-    (*order->item)->marker=1;		/* Mark found */
-    if ((*order->item)->with_sum_func)
+    (*ord->item)->marker= UNDEF_POS;		/* Mark found */
+    if ((*ord->item)->with_sum_func)
     {
-      my_error(ER_WRONG_GROUP_FIELD, MYF(0), (*order->item)->full_name());
+      my_error(ER_WRONG_GROUP_FIELD, MYF(0), (*ord->item)->full_name());
       return 1;
     }
   }
   if (thd->variables.sql_mode & MODE_ONLY_FULL_GROUP_BY)
   {
-    /* Don't allow one to use fields that is not used in GROUP BY */
+    /*
+      Don't allow one to use fields that is not used in GROUP BY
+      For each select a list of field references that aren't under an
+      aggregate function is created. Each field in this list keeps the
+      position of the select list expression which it belongs to.
+
+      First we check an expression from the select list against the GROUP BY
+      list. If it's found there then it's ok. It's also ok if this expression
+      is a constant or an aggregate function. Otherwise we scan the list
+      of non-aggregated fields and if we'll find at least one field reference
+      that belongs to this expression and doesn't occur in the GROUP BY list
+      we throw an error. If there are no fields in the created list for a
+      select list expression this means that all fields in it are used under
+      aggregate functions.
+    */
     Item *item;
+    Item_field *field;
+    int cur_pos_in_select_list= 0;
     List_iterator<Item> li(fields);
+    List_iterator<Item_field> naf_it(thd->lex->current_select->non_agg_fields);
 
-    while ((item=li++))
+    field= naf_it++;
+    while (field && (item=li++))
     {
-      if (item->type() != Item::SUM_FUNC_ITEM && !item->marker &&
-	  !item->const_item())
+      if (item->type() != Item::SUM_FUNC_ITEM && item->marker >= 0 &&
+          !item->const_item() &&
+          !(item->real_item()->type() == Item::FIELD_ITEM &&
+            item->used_tables() & OUTER_REF_TABLE_BIT))
       {
-        /*
-          TODO: change ER_WRONG_FIELD_WITH_GROUP to more detailed
-          ER_NON_GROUPING_FIELD_USED
-        */
-	my_error(ER_WRONG_FIELD_WITH_GROUP, MYF(0), item->full_name());
-	return 1;
+        while (field)
+        {
+          /* Skip fields from previous expressions. */
+          if (field->marker < cur_pos_in_select_list)
+            goto next_field;
+          /* Found a field from the next expression. */
+          if (field->marker > cur_pos_in_select_list)
+            break;
+          /*
+            Check whether the field occur in the GROUP BY list.
+            Throw the error later if the field isn't found.
+          */
+          for (ord= order; ord; ord= ord->next)
+            if ((*ord->item)->eq((Item*)field, 0))
+              goto next_field;
+          /*
+            TODO: change ER_WRONG_FIELD_WITH_GROUP to more detailed
+            ER_NON_GROUPING_FIELD_USED
+          */
+          my_error(ER_WRONG_FIELD_WITH_GROUP, MYF(0), field->full_name());
+          return 1;
+next_field:
+          field= naf_it++;
+        }
       }
+      cur_pos_in_select_list++;
     }
   }
   if (org_fields != all_fields.elements)

sql/sql_union.cc

@@ -732,6 +732,7 @@ bool st_select_lex::cleanup()
   {
     error= (bool) ((uint) error | (uint) lex_unit->cleanup());
   }
+  non_agg_fields.empty();
   DBUG_RETURN(error);
 }