[Maria-developers] Review form MDEV-11297 patch

30 Nov 2016

Hi Varun,

Please find my review comments below. We'll need another review pass once
these are addressed.
...
commit e9d1969a9758801674828f26431306350bb4c0ab
Author: Varun <varunraiko1803@gmail.com>
Date:   Mon Nov 28 15:09:03 2016 +0530
LIMIT Clause without optimisation added to GROUP_CONCAT
Please mention MDEV-11297 in the commit comment. The idea is that it should be
possible to search for MDEV number in 'git log' output and find all commits
with the code for the issue.
...

diff --git a/mysql-test/t/group_concat_limit.test b/mysql-test/t/group_concat_limit.test
index 82baf54..7230e80 100644
Ok this is .test file. Where is the .result file, did you forget to add it?

Please also add testcases where the GROUP_CONCAT's argument is NULL.
...
--- a/mysql-test/t/group_concat_limit.test
+++ b/mysql-test/t/group_concat_limit.test
@@ -18,30 +18,30 @@ insert into t1 values (3,7,"E","c");
# Test of MySQL simple request
-select grp,group_concat(c limit 1 ) from t1 group by grp;
+select grp,group_concat(c limit 0 ) from t1 group by grp;
 select grp,group_concat(c limit 1,1 ) from t1 group by grp;
select grp,group_concat(c limit 1,10 ) from t1 group by grp;
 select grp,group_concat(distinct c limit 1,10 ) from t1 group by grp;
select grp,group_concat(c order by a) from t1 group by grp;
-elect grp,group_concat(c order by a limit 2 ) from t1 group by grp;
+select grp,group_concat(c order by a limit 2 ) from t1 group by grp;
select grp,group_concat(c order by a limit 1,1 ) from t1 group by grp;
 select grp,group_concat(c order by a limit 1,2 ) from t1 group by grp;
 select grp,group_concat(c order by a limit 10 ) from t1 group by grp;
-#select grp,group_concat(c order by c) from t1 group by grp;
-#select grp,group_concat(c order by c limit 2) from t1 group by grp;
+select grp,group_concat(c order by c) from t1 group by grp;
+select grp,group_concat(c order by c limit 2) from t1 group by grp;
-#select grp,group_concat(c order by c desc) from t1 group by grp;
-#select grp,group_concat(c order by c desc limit 2) from t1 group by grp;
+select grp,group_concat(c order by c desc) from t1 group by grp;
+select grp,group_concat(c order by c desc limit 2) from t1 group by grp;
-#select grp,group_concat(d order by a) from t1 group by grp;
-#select grp,group_concat(d order by a limit 2) from t1 group by grp;
+select grp,group_concat(d order by a) from t1 group by grp;
+select grp,group_concat(d order by a limit 2) from t1 group by grp;
-#select grp,group_concat(d order by a desc) from t1 group by grp;
-#select grp,group_concat(d order by a desc limit 1) from t1 group by grp;
+select grp,group_concat(d order by a desc) from t1 group by grp;
+select grp,group_concat(d order by a desc limit 1) from t1 group by grp;
What about this:

create table t2 (a int, b varchar(10));
insert into t2 values 
  (1,'a'),(1,'b'),(1,'c'),
  (2,'x'),(2,'y');
MariaDB [j10]> select group_concat(a,b) from t2;
+-------------------+
| group_concat(a,b) |
+-------------------+
| 1a,1b,1c,2x,2y    |
+-------------------+
1 row in set (0.00 sec)

MariaDB [j10]> select group_concat(a,b limit 3) from t2;
+---------------------------+
| group_concat(a,b limit 3) |
+---------------------------+
| 1a,1                      |
+---------------------------+
1 row in set (0.01 sec)

Do you think it's correct? We need to discuss this.
...
drop table t1;
diff --git a/sql/item_sum.cc b/sql/item_sum.cc
index 51a6c2b..681b800 100644
--- a/sql/item_sum.cc
+++ b/sql/item_sum.cc
@@ -3114,6 +3114,11 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)),
   String *result= &item->result;
   Item **arg= item->args, **arg_end= item->args + item->arg_count_field;
   uint old_length= result->length();
+  ulonglong *offset_limit= &item->offset_limit;
+  ulonglong *row_limit = &item->row_limit;
+
+  if(item->limit_clause && !(*row_limit))
+     return 1;
if (item->no_appended)
     item->no_appended= FALSE;
@@ -3148,7 +3153,31 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)),
         res= (*arg)->val_str(&tmp);
     }
     if (res)
-      result->append(*res);
+    {
+      // This can be further optimised if we calculate the values for the fields only when it is necessary that is afer #offset_limit
+      if(item->limit_clause)
+      {
+        if(*offset_limit)
+        {
+          (*offset_limit)--;
+          item->no_appended= TRUE;
+        }
+        else
+        {
+          if(*row_limit)
+          {
+            result->append(*res);
+            (*row_limit)--;
+            if(!(*row_limit))
+              item->no_appended= TRUE;
I've commented out the above two lines and the testcase still produces the
same result.
I claim that they are not needed. If you think otherwise, please add test
coverage.
Please do the same for the assignment right below.
...
+          }
+          else
+            item->no_appended= TRUE;
+        }
+      }
+      else
+        result->append(*res);
+    }
   }
item->row_count++;
@@ -3199,7 +3228,8 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)),
 Item_func_group_concat(THD *thd, Name_resolution_context *context_arg,
                        bool distinct_arg, List<Item> *select_list,
                        const SQL_I_List<ORDER> &order_list,
-                       String *separator_arg)
+                       String *separator_arg, bool limit_clause,
+                       ulonglong row_limit, ulonglong offset_limit)
   :Item_sum(thd), tmp_table_param(0), separator(separator_arg), tree(0),
    unique_filter(NULL), table(0),
    order(0), context(context_arg),
@@ -3208,7 +3238,9 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)),
    row_count(0),
    distinct(distinct_arg),
    warning_for_row(FALSE),
-   force_copy_fields(0), original(0)
+   force_copy_fields(0), original(0),
+   row_limit(row_limit), offset_limit(offset_limit),limit_clause(limit_clause),
+   copy_offset_limit(offset_limit), copy_row_limit(row_limit)
 {
   Item *item_select;
   Item **arg_ptr;
@@ -3269,7 +3301,9 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)),
   warning_for_row(item->warning_for_row),
   always_null(item->always_null),
   force_copy_fields(item->force_copy_fields),
-  original(item)
+  original(item), row_limit(item->row_limit),
+  offset_limit(item->offset_limit),limit_clause(item->limit_clause),
+  copy_offset_limit(item->copy_offset_limit), copy_row_limit(item->row_limit)
 {
   quick_group= item->quick_group;
   result.set_charset(collation.collation);
@@ -3382,6 +3416,8 @@ void Item_func_group_concat::clear()
   null_value= TRUE;
   warning_for_row= FALSE;
   no_appended= TRUE;
+  offset_limit= copy_offset_limit;
+  row_limit= copy_row_limit;
   if (tree)
     reset_tree(tree);
   if (unique_filter)
diff --git a/sql/item_sum.h b/sql/item_sum.h
index b9075db..a38c5ca 100644
--- a/sql/item_sum.h
+++ b/sql/item_sum.h
@@ -1583,6 +1583,12 @@ class Item_func_group_concat : public Item_sum
   bool always_null;
   bool force_copy_fields;
   bool no_appended;
+  bool limit_clause;
+  ulonglong row_limit;
+  ulonglong offset_limit;
+  ulonglong copy_offset_limit;
+  ulonglong copy_row_limit;
Please add (short) comments describing each new member variable.
...
+
   /*
     Following is 0 normal object and pointer to original one for copy
     (to correctly free resources)
@@ -1602,7 +1608,8 @@ class Item_func_group_concat : public Item_sum
 public:
   Item_func_group_concat(THD *thd, Name_resolution_context *context_arg,
                          bool is_distinct, List<Item> *is_select,
-                         const SQL_I_List<ORDER> &is_order, String *is_separator);
+                         const SQL_I_List<ORDER> &is_order, String *is_separator,
+                         bool limit_clause, ulonglong row_limit, ulonglong offset_limit);
Item_func_group_concat(THD *thd, Item_func_group_concat *item);
   ~Item_func_group_concat();
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
index 163322d..d87e6fb 100644
--- a/sql/sql_yacc.yy
+++ b/sql/sql_yacc.yy
@@ -1786,7 +1786,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize);
 %type <num>
         order_dir lock_option
         udf_type opt_local opt_no_write_to_binlog
-        opt_temporary all_or_any opt_distinct
+        opt_temporary all_or_any opt_distinct opt_glimit_clause
         opt_ignore_leaves fulltext_options union_option
         opt_not
         select_derived_init transaction_access_mode_types
@@ -1844,7 +1844,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize);
         opt_escape
         sp_opt_default
         simple_ident_nospvar simple_ident_q
-        field_or_var limit_option
+        field_or_var limit_option glimit_option
         part_func_expr
         window_func_expr
         window_func
@@ -10449,16 +10449,36 @@ sum_expr:
         | GROUP_CONCAT_SYM '(' opt_distinct
           { Select->in_sum_expr++; }
           expr_list opt_gorder_clause
-          opt_gconcat_separator
+          opt_gconcat_separator opt_glimit_clause
           ')'
           {
             SELECT_LEX *sel= Select;
             sel->in_sum_expr--;
-            $$= new (thd->mem_root)
-                  Item_func_group_concat(thd, Lex->current_context(), $3, $5,
-                                         sel->gorder_list, $7);
-            if ($$ == NULL)
-              MYSQL_YYABORT;
+            if(!$8)
Coding style requires a space after "if":  "if (...)".  Please fix this
everywhere in this patch.
IIRC it also requires that if the statement inside if, it should be enclosed
in {...}.
...
+             $$= new (thd->mem_root)
+                   Item_func_group_concat(thd, Lex->current_context(), $3, $5,
+                                        sel->gorder_list, $7, FALSE, 0, 0);
+            else
+            {
+              if(sel->select_limit && sel->offset_limit)
+                 $$= new (thd->mem_root)
+                        Item_func_group_concat(thd, Lex->current_context(), $3, $5,
+                                         sel->gorder_list, $7, TRUE, sel->select_limit->val_int(),
+                                         sel->offset_limit->val_int());
There is at least one case where you can't call val_int from the parser: when
LIMIT number is a prepared statement parameter.

consider an example:
create table t2 (a int, b varchar(10));
insert into t2 values 
  (1,'a'),(1,'b'),(1,'c'),
  (2,'x'),(2,'y');

prepare STMT from 'select group_concat(b order by a limit ?) from t2' ;

This crashes:

  Program received signal SIGABRT, Aborted.
  0x00007ffff5d7a425 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
  64	../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.

  #2  0x00007ffff5d730ee in __assert_fail_base (fmt=<optimized out>, assertion=0x5555566d8bd7 "0", file=0x5555566d8f08 "/home/psergey/dev-git/10.2-varun-mdev11297/sql/item.cc", line=<optimized out>, function=<optimized out>) at assert.c:94
  #3  0x00007ffff5d73192 in __GI___assert_fail (assertion=0x5555566d8bd7 "0", file=0x5555566d8f08 "/home/psergey/dev-git/10.2-varun-mdev11297/sql/item.cc", line=3689, function=0x5555566dbd60 "virtual longlong Item_param::val_int()") at assert.c:103
  #4  0x0000555555de54a0 in Item_param::val_int (this=0x7fff40054160) at /home/psergey/dev-git/10.2-varun-mdev11297/sql/item.cc:3689
  #5  0x0000555555d63277 in MYSQLparse (thd=0x7fff40000b00) at /home/psergey/dev-git/10.2-varun-mdev11297/sql/sql_yacc.yy:10471
  #6  0x0000555555b6e696 in parse_sql (thd=0x7fff40000b00, parser_state=0x7fffdc0c8cb0, creation_ctx=0x0, do_pfs_digest=false) at /home/psergey/dev-git/10.2-varun-mdev11297/sql/sql_parse.cc:9815
  #7  0x0000555555b848b2 in Prepared_statement::prepare (this=0x7fff4004bd80, packet=0x7fff400133a0 "select group_concat(b order by a limit ?) from t2", packet_len=49) at /home/psergey/dev-git/10.2-varun-mdev11297/sql/sql_prepare.cc:3835
...
...
+              else if(sel->select_limit)
+                 $$= new (thd->mem_root)
+                        Item_func_group_concat(thd, Lex->current_context(), $3, $5,
+                                         sel->gorder_list, $7, TRUE, sel->select_limit->val_int(),0);
+              else
+                 $$ = NULL; //need more thinking and maybe some error handling to inform the user the problem with the query
First, the identation is wrong, this 'else' is actually attached to the 
"if(sel->select_limit)".  One doesn't make such mistakes when they are 
using {...} for all multi-line if-elses.
Second, can the "$$ = NULL" ever execute at all? It will be run when the LIMIT
clause is present (as $8!=0), but neither LIMIT nor OFFSET clauses are. I think 
the grammar prevents that.
You can add an assert if you still want the code here to cover all cases.
...
+            }
+             if ($$ == NULL)
+               MYSQL_YYABORT;
+
+            sel->select_limit= NULL;
+            sel->offset_limit= NULL;
+            sel->explicit_limit= 0;
+
             $5->empty();
             sel->gorder_list.empty();
           }
@@ -10680,6 +10700,105 @@ gorder_list:
           { if (add_gorder_to_list(thd, $1,(bool) $2)) MYSQL_YYABORT; }
         ;
+opt_glimit_clause:
+          /* empty */ { $$ = 0; }
+        | glimit_clause { $$ = 1; }
+        ;
+
+glimit_clause_init:
+          LIMIT{}
+        ;
+
+glimit_clause:
+          glimit_clause_init glimit_options{}
+        | glimit_clause_init glimit_options
+          ROWS_SYM EXAMINED_SYM glimit_rows_option{}
+        | glimit_clause_init ROWS_SYM EXAMINED_SYM glimit_rows_option{}
+        ;
+
+glimit_options:
+          glimit_option
+          {
+            SELECT_LEX *sel= Select;
+            sel->select_limit= $1;
+            sel->offset_limit= 0;
+            sel->explicit_limit= 1;
+          }
+        | glimit_option ',' glimit_option
+          {
+            SELECT_LEX *sel= Select;
+            sel->select_limit= $3;
+            sel->offset_limit= $1;
+            sel->explicit_limit= 1;
+          }
+        | glimit_option OFFSET_SYM glimit_option
+          {
+            SELECT_LEX *sel= Select;
+            sel->select_limit= $1;
+            sel->offset_limit= $3;
+            sel->explicit_limit= 1;
+          }
+        ;
+
+glimit_option:
+        ident
+        {
+          Item_splocal *splocal;
+          LEX *lex= thd->lex;
+          Lex_input_stream *lip= & thd->m_parser_state->m_lip;
+          sp_variable *spv;
+          sp_pcontext *spc = lex->spcont;
+          if (spc && (spv = spc->find_variable($1, false)))
+          {
+            splocal= new (thd->mem_root)
+              Item_splocal(thd, $1, spv->offset, spv->sql_type(),
+                  lip->get_tok_start() - lex->sphead->m_tmp_query,
+                  lip->get_ptr() - lip->get_tok_start());
+            if (splocal == NULL)
+              MYSQL_YYABORT;
+#ifndef DBUG_OFF
+            splocal->m_sp= lex->sphead;
+#endif
+            lex->safe_to_cache_query=0;
+          }
+          else
+            my_yyabort_error((ER_SP_UNDECLARED_VAR, MYF(0), $1.str));
+          if (splocal->type() != Item::INT_ITEM)
+            my_yyabort_error((ER_WRONG_SPVAR_TYPE_IN_LIMIT, MYF(0)));
+          splocal->limit_clause_param= TRUE;
+          $$= splocal;
+        }
+        | param_marker
+        {
+          $1->limit_clause_param= TRUE;
+        }
+        | ULONGLONG_NUM
+          {
+            $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length);
+            if ($$ == NULL)
+              MYSQL_YYABORT;
+          }
+        | LONG_NUM
+          {
+            $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length);
+            if ($$ == NULL)
+              MYSQL_YYABORT;
+          }
+        | NUM
+          {
+            $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length);
+            if ($$ == NULL)
+              MYSQL_YYABORT;
+          }
+        ;
+
+glimit_rows_option:
+          glimit_option
+          {
+            LEX *lex=Lex;
+            lex->limit_rows_examined= $1;
+          }
+
 in_sum_expr:
           opt_all
           {
When compiling, I'm getting warnings like this:

|| [ 76%] Building CXX object sql/CMakeFiles/sql.dir/item_sum.cc.o
|| /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h: In constructor ‘Item_func_group_concat::Item_func_group_concat(THD*, Name_resolution_context*, bool, List<Item>*, const SQL_I_List<st_order>&, String*, bool, ulonglong, ulonglong)’:
/home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1596 col 27| warning: ‘Item_func_group_concat::original’ will be initialized after [-Wreorder]
/home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1587 col 13| warning:   ‘ulonglong Item_func_group_concat::row_limit’ [-Wreorder]
item_sum.cc|3227 col 1| warning:   when initialized here [-Wreorder]
/home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1588 col 13| warning: ‘Item_func_group_concat::offset_limit’ will be initialized after [-Wreorder]
/home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1586 col 8| warning:   ‘bool Item_func_group_concat::limit_clause’ [-Wreorder]
item_sum.cc|3227 col 1| warning:   when initialized here [-Wreorder]
|| /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h: In constructor ‘Item_func_group_concat::Item_func_group_concat(THD*, Item_func_group_concat*)’:
/home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1596 col 27| warning: ‘Item_func_group_concat::original’ will be initialized after [-Wreorder]
/home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1587 col 13| warning:   ‘ulonglong Item_func_group_concat::row_limit’ [-Wreorder]
item_sum.cc|3288 col 1| warning:   when initialized here [-Wreorder]
/home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1588 col 13| warning: ‘Item_func_group_concat::offset_limit’ will be initialized after [-Wreorder]
/home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1586 col 8| warning:   ‘bool Item_func_group_concat::limit_clause’ [-Wreorder]
item_sum.cc|3288 col 1| warning:   when initialized here [-Wreorder]
|| Linking CXX static library libsql.a




BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

    

[Maria-developers] Review form MDEV-11297 patch

Sergey Petrunia