Implementation of the last review comments for WL#1724

"Min/Max Optimization for Queries with Group By Clause"


mysql-test/r/group_min_max.result:
  Added new test
mysql-test/t/group_min_max.test:
  Added new test & comments
sql/item.h:
  Remove processor methods that are no longer used.
sql/item_sum.cc:
  Remove processor methods that are no longer used.
sql/item_sum.h:
  Remove processor methods that are no longer used.
sql/opt_range.cc:
  - the test procedure no longer needs to build lists and to traverse the expression trees
  - all usable indexes are tested and the bes one is chosen
  - added/edited function comments
sql/sql_select.cc:
  Modified make_sum_func_list() so it can be reused in the test for MIN/MAX optimization.
sql/sql_select.h:
  Modified make_sum_func_list() so it can be reused in the test for MIN/MAX optimization.

mysql-test/r/group_min_max.result

@@ -1869,6 +1869,12 @@ id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
 explain select a1,a2,b from t1 where (a1 = 'b' or a1 = 'd' or a1 = 'a' or a1 = 'c') and (a2 > 'a') and (c > 'a111') group by a1,a2,b;
 id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
 1	SIMPLE	t1	range	idx_t1_0,idx_t1_1,idx_t1_2	idx_t1_1	130	NULL	76	Using where; Using index
+explain select a1,a2,min(b),c from t2 where (a2 = 'a') and (c = 'a111') group by a1;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	SIMPLE	t2	index	NULL	idx_t2_1	163	NULL	164	Using where; Using index
+select a1,a2,min(b),c from t2 where (a2 = 'a') and (c = 'a111') group by a1;
+a1	a2	min(b)	c
+a	a	a	a111
 explain select a1,a2,b,max(c),min(c) from t2 where (a2 = 'a') and (b = 'b') or (b = 'a') group by a1;
 id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
 1	SIMPLE	t2	index	NULL	idx_t2_1	163	NULL	164	Using where; Using index

mysql-test/t/group_min_max.test

@@ -3,6 +3,12 @@
 # The queries in this file test query execution via QUICK_GROUP_MIN_MAX_SELECT.
 #
 
+#
+# TODO:
+# Add queries with:
+# - C != const
+# - C IS NOT NULL
+# - HAVING clause
 
 --disable_warnings
 drop table if exists t1;
@@ -175,8 +181,6 @@ explain select a1, b, min(c), a1, max(c), b, a2, max(c), max(c) from t1 group by
 explain select min(a2) from t1 group by a1;
 explain select a2, min(c), max(c) from t1 group by a1,a2,b;
 
--- TODO: Queries with HAVING
-
 -- queries
 select a1, min(a2) from t1 group by a1;
 select a1, max(a2) from t1 group by a1;
@@ -190,8 +194,6 @@ select a1, b, min(c), a1, max(c), b, a2, max(c), max(c) from t1 group by a1, a2,
 select min(a2) from t1 group by a1;
 select a2, min(c), max(c) from t1 group by a1,a2,b;
 
--- TODO: Queries with HAVING
-
 --
 -- Queries with a where clause
 --
@@ -300,7 +302,6 @@ select a1,a2,b,min(c) from t2 where b is NULL group by a1,a2;
 select a1,a2,b,max(c) from t2 where b is NULL group by a1,a2;
 select a1,a2,b,min(c),max(c) from t2 where b is NULL group by a1,a2;
 select a1,a2,b,min(c),max(c) from t2 where b is NULL group by a1,a2;
--- TODO: IS NOT NULL ?
 
 -- C) Range predicates for the MIN/MAX attribute
 -- plans
@@ -553,6 +554,11 @@ where (a1 = 'b' or a1 = 'd' or a1 = 'a' or a1 = 'c') and (a2 > 'a') and (d > 'xy
 explain select a1,a2,b,max(c),min(c) from t2 where (a2 = 'a') and (b = 'b') or (b < 'b') group by a1;
 explain select a1,a2,b from t1 where (a1 = 'b' or a1 = 'd' or a1 = 'a' or a1 = 'c') and (a2 > 'a') and (c > 'a111') group by a1,a2,b;
 
+-- non-group field with an equality predicate that references a keypart after the
+-- MIN/MAX argument
+explain select a1,a2,min(b),c from t2 where (a2 = 'a') and (c = 'a111') group by a1;
+select a1,a2,min(b),c from t2 where (a2 = 'a') and (c = 'a111') group by a1;
+
 -- disjunction for a non-group select attribute
 explain select a1,a2,b,max(c),min(c) from t2 where (a2 = 'a') and (b = 'b') or (b = 'a') group by a1;
 

sql/item.h

@@ -260,22 +260,7 @@ public:
 
   virtual bool remove_dependence_processor(byte * arg) { return 0; }
   virtual bool remove_fixed(byte * arg) { fixed= 0; return 0; }
-  /*
-    All collect_* methods are used as arguments to walk() to collect
-    specific types items.
-    TODO:
-    A more generic implementation would add a special class
-    Collect_processor_param that can store arbitrary sets of item kinds
-    (currently specified as enums), along with a list to store items of the
-    specified kinds. This would allow to collect combinations of items of
-    arbitrary kinds without having to add a new collect method each time.
-    There can be one generic collect_processor method that checks the item type
-    and compares it with the item types in Collect_processor_param.
-  */
   virtual bool collect_item_field_processor(byte * arg) { return 0; }
-  virtual bool collect_item_sum_min_processor(byte * arg) { return 0; }
-  virtual bool collect_item_sum_max_processor(byte * arg) { return 0; }
-  virtual bool has_non_min_max_sum_processor(byte * arg) { return 0; }
   
   virtual Item *this_item() { return this; } /* For SPs mostly. */
   virtual Item *this_const_item() const { return const_cast<Item*>(this); } /* For SPs mostly. */

sql/item_sum.cc

@@ -183,44 +183,6 @@ bool Item_sum::walk (Item_processor processor, byte *argument)
 }
 
 
-/*
-  Store the pointer to this item into a list if not already there.
-
-  SYNOPSIS
-    Item_sum::collect()
-    item_list  pointer to a List<Item_sum> where item_sum objects are collected
-
-  DESCRIPTION
-    The method is used by collect_item_sum_*_processor, called by
-    Item_sum::walk, to collect all unique Item_sum_min and Item_sum_max objects
-    from a tree of Items into a set of items represented as a list.
-
-  IMPLEMENTATION
-    Item_cond::walk() and Item_func::walk() stop the evaluation of the
-    processor function for its arguments once the processor returns
-    true.Therefore in order to force this method being called for all item
-    arguments in a condition the method must return false.
-
-  RETURN
-    FALSE on success (force the evaluation of collect_item_sum_*_processor
-          for the subsequent items.)
-    TRUE  o/w (stop evaluation of subsequent items.)
-*/
-
-bool Item_sum::collect(List<Item_sum> *item_list)
-{
-  List_iterator<Item_sum> item_list_it(*item_list);
-  Item_sum *curr_item;
-  while ((curr_item= item_list_it++))
-  {
-    if (curr_item == this)
-      return FALSE; /* Already in the set. */
-  }
-  item_list->push_back(this);
-  return FALSE;
-}
-
-
 String *
 Item_sum_num::val_str(String *str)
 {

sql/item_sum.h

@@ -27,8 +27,6 @@ class Item_arena;
 
 class Item_sum :public Item_result_field
 {
-private:
-  bool collect(List<Item_sum> *item_list);
 public:
   enum Sumfunctype
   { COUNT_FUNC, COUNT_DISTINCT_FUNC, SUM_FUNC, SUM_DISTINCT_FUNC, AVG_FUNC,
@@ -100,33 +98,6 @@ public:
   bool save_args(Item_arena* stmt);
 
   bool walk (Item_processor processor, byte *argument);
-
-  /* Collect Item_sum_min objects into a list supplied by the caller. */
-  bool collect_item_sum_min_processor(byte *arg)
-  {
-    if (Item_sum::MIN_FUNC == this->sum_func())
-      return collect((List<Item_sum>*) arg);
-    else
-      return FALSE;
-  }
-
-  /* Collect Item_sum_max objects into a list supplied by the caller. */
-  bool collect_item_sum_max_processor(byte *arg)
-  {
-    if (Item_sum::MAX_FUNC == this->sum_func())
-      return collect((List<Item_sum>*) arg);
-    else
-      return FALSE;
-   }
-
-  /* Check if there are any aggregate functions other than MIN and MAX. */
-  bool has_non_min_max_sum_processor(byte * arg)
-  {
-    Sumfunctype sum_type= this->sum_func();
-    if ((sum_type != Item_sum::MIN_FUNC) && (sum_type != Item_sum::MAX_FUNC))
-      return TRUE;
-    return FALSE;
-  }
 };
 
 

sql/sql_select.cc

@@ -1327,7 +1327,7 @@ JOIN::exec()
 	}
       }
       if (curr_join->make_sum_func_list(*curr_all_fields, *curr_fields_list,
-					1) ||
+					1, TRUE) ||
 	  (tmp_error= do_select(curr_join, (List<Item> *) 0, curr_tmp_table,
 				0)))
       {
@@ -1415,7 +1415,7 @@ JOIN::exec()
     set_items_ref_array(items3);
 
     if (curr_join->make_sum_func_list(*curr_all_fields, *curr_fields_list,
-				      1) || thd->is_fatal_error)
+				      1, TRUE) || thd->is_fatal_error)
       DBUG_VOID_RETURN;
   }
   if (curr_join->group_list || curr_join->order)
@@ -10765,6 +10765,7 @@ bool JOIN::alloc_func_list()
     field_list		All items
     send_fields		Items in select list
     before_group_by	Set to 1 if this is called before GROUP BY handling
+    recompute           Set to TRUE if sum_funcs must be recomputed
 
   NOTES
     Calls ::setup() for all item_sum objects in field_list
@@ -10775,13 +10776,16 @@ bool JOIN::alloc_func_list()
 */
 
 bool JOIN::make_sum_func_list(List<Item> &field_list, List<Item> &send_fields,
-			      bool before_group_by)
+			      bool before_group_by, bool recompute)
 {
   List_iterator_fast<Item> it(field_list);
   Item_sum **func;
   Item *item;
   DBUG_ENTER("make_sum_func_list");
 
+  if (*sum_funcs && !recompute)
+    DBUG_RETURN(FALSE); /* We have already initialized sum_funcs. */
+
   func= sum_funcs;
   while ((item=it++))
   {

sql/sql_select.h

@@ -303,7 +303,7 @@ class JOIN :public Sql_alloc
   void restore_tmp();
   bool alloc_func_list();
   bool make_sum_func_list(List<Item> &all_fields, List<Item> &send_fields,
-			  bool before_group_by);
+			  bool before_group_by, bool recompute= FALSE);
 
   inline void set_items_ref_array(Item **ptr)
   {