MDEV-21879 GROUP_CONCAT(DISTINCT ORDER BY) is wrong when Unique spills

mysql-test/main/func_gconcat.result

@@ -863,7 +863,7 @@ group_concat(distinct a, c)
 00,01,10,11,31
 select group_concat(distinct a, c order by a) from t1;
 group_concat(distinct a, c order by a)
-00,01,11,10,31
+01,00,11,10,31
 select group_concat(distinct a, c) from t1;
 group_concat(distinct a, c)
 00,01,10,11,31

mysql-test/main/gconcat_distinct_spill.result

@@ -0,0 +1,105 @@
+#
+# Each block records the answer computed with memory to spare, then
+# recomputes it with the duplicate filter starved, and compares.
+#
+CREATE TABLE t1 (pk INT AUTO_INCREMENT PRIMARY KEY, a VARCHAR(100) NOT NULL);
+INSERT INTO t1 (a) SELECT LPAD(seq, 4, '0') FROM seq_1_to_50;
+INSERT INTO t1 (a) SELECT a FROM t1 ORDER BY pk;
+SELECT COUNT(*) AS rows_in_table, COUNT(DISTINCT a) AS distinct_values FROM t1;
+rows_in_table	distinct_values
+100	50
+SELECT GROUP_CONCAT(DISTINCT a) INTO @gc FROM t1;
+SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc_order FROM t1;
+SELECT JSON_ARRAYAGG(DISTINCT a) INTO @ja FROM t1;
+SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) INTO @ja_order FROM t1;
+SET @@tmp_memory_table_size=0;
+SELECT GROUP_CONCAT(DISTINCT a) = @gc AS gc_unchanged FROM t1;
+gc_unchanged
+1
+SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) = @gc_order AS gc_order_unchanged FROM t1;
+gc_order_unchanged
+1
+SELECT JSON_ARRAYAGG(DISTINCT a) = @ja AS ja_unchanged FROM t1;
+ja_unchanged
+1
+SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) = @ja_order AS ja_order_unchanged FROM t1;
+ja_order_unchanged
+1
+SET @@tmp_memory_table_size=DEFAULT;
+DROP TABLE t1;
+#
+# Values wide enough that the filter flushes on nearly every row.
+# Here the ORDER BY case used to return a single value out of 30.
+#
+CREATE TABLE t2 (a VARCHAR(2000)) AS
+SELECT CONCAT(seq, REPEAT('.', 1990)) AS a FROM seq_1_to_30;
+SELECT COUNT(*) AS rows_in_table, COUNT(DISTINCT a) AS distinct_values FROM t2;
+rows_in_table	distinct_values
+30	30
+SELECT GROUP_CONCAT(DISTINCT a) INTO @gc FROM t2;
+SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc_order FROM t2;
+SET @@tmp_memory_table_size=1000, @@max_heap_table_size=1000;
+Warnings:
+Warning	1292	Truncated incorrect tmp_memory_table_size value: '1000'
+Warning	1292	Truncated incorrect max_heap_table_size value: '1000'
+SELECT GROUP_CONCAT(DISTINCT a) = @gc AS gc_unchanged FROM t2;
+gc_unchanged
+1
+SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) = @gc_order AS gc_order_unchanged FROM t2;
+gc_order_unchanged
+1
+SET @@tmp_memory_table_size=DEFAULT, @@max_heap_table_size=DEFAULT;
+DROP TABLE t2;
+#
+# ORDER BY does not order the rows that tie on the ordering
+# expression. Which of them comes first is not specified, but it
+# must not depend on the memory available or on the order the rows
+# are read in.
+#
+CREATE TABLE t3 (a BIT(2), b VARCHAR(10), c BIT);
+INSERT INTO t3 VALUES (1, 'a', 0), (0, 'b', 1), (0, 'c', 0), (3, 'd', 1),
+(1, 'e', 1), (3, 'f', 1), (0, 'g', 1);
+SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS at_default FROM t3;
+at_default
+01,00,11,10,31
+SET @@tmp_memory_table_size=0;
+SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS at_zero FROM t3;
+at_zero
+01,00,11,10,31
+SET @@tmp_memory_table_size=DEFAULT;
+DELETE FROM t3;
+INSERT INTO t3 VALUES (0, 'c', 0), (0, 'b', 1), (1, 'a', 0), (3, 'd', 1),
+(1, 'e', 1), (3, 'f', 1), (0, 'g', 1);
+SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS reversed_scan_order FROM t3;
+reversed_scan_order
+01,00,11,10,31
+DROP TABLE t3;
+#
+# The sort tree can overflow too. Starving it makes repack_tree()
+# cut rows out of the group, which is expected, but the rows that
+# do come back must still be deduplicated and still be in order.
+#
+CREATE TABLE t4 (pk INT AUTO_INCREMENT PRIMARY KEY, a VARCHAR(20));
+INSERT INTO t4 (a) SELECT LPAD(seq, 6, '0') FROM seq_1_to_200;
+INSERT INTO t4 (a) SELECT a FROM t4 ORDER BY pk;
+SET @@tmp_memory_table_size=0, @@group_concat_max_len=4000;
+SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc FROM t4;
+Warnings:
+Warning	1260	Row 32 was cut by group_concat()
+SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) INTO @ja FROM t4;
+SET @@tmp_memory_table_size=DEFAULT, @@group_concat_max_len=DEFAULT;
+SELECT COUNT(*) = COUNT(DISTINCT val) AS gc_no_duplicates,
+GROUP_CONCAT(val ORDER BY val) = @gc AS gc_ascending
+FROM (SELECT SUBSTRING_INDEX(SUBSTRING_INDEX(@gc, ',', seq), ',', -1) AS val
+FROM seq_1_to_500
+WHERE seq <= 1 + LENGTH(@gc) - LENGTH(REPLACE(@gc, ',', ''))) split;
+gc_no_duplicates	gc_ascending
+1	1
+SELECT JSON_VALID(@ja) AS ja_valid,
+COUNT(*) = COUNT(DISTINCT val) AS ja_no_duplicates,
+JSON_ARRAYAGG(val ORDER BY val) = @ja AS ja_ascending
+FROM (SELECT JSON_UNQUOTE(JSON_EXTRACT(@ja, CONCAT('$[', seq - 1, ']'))) AS val
+FROM seq_1_to_500 WHERE seq <= JSON_LENGTH(@ja)) split;
+ja_valid	ja_no_duplicates	ja_ascending
+1	1	1
+DROP TABLE t4;

mysql-test/main/gconcat_distinct_spill.test

@@ -0,0 +1,93 @@
+#
+# GROUP_CONCAT(DISTINCT ...) and JSON_ARRAYAGG(DISTINCT ...) filter
+# duplicates with a Unique object, which flushes to disk when it runs out
+# of memory. The answer must not depend on whether that flush happened.
+#
+--source include/have_sequence.inc
+
+--echo #
+--echo # Each block records the answer computed with memory to spare, then
+--echo # recomputes it with the duplicate filter starved, and compares.
+--echo #
+
+CREATE TABLE t1 (pk INT AUTO_INCREMENT PRIMARY KEY, a VARCHAR(100) NOT NULL);
+INSERT INTO t1 (a) SELECT LPAD(seq, 4, '0') FROM seq_1_to_50;
+INSERT INTO t1 (a) SELECT a FROM t1 ORDER BY pk;
+SELECT COUNT(*) AS rows_in_table, COUNT(DISTINCT a) AS distinct_values FROM t1;
+
+SELECT GROUP_CONCAT(DISTINCT a) INTO @gc FROM t1;
+SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc_order FROM t1;
+SELECT JSON_ARRAYAGG(DISTINCT a) INTO @ja FROM t1;
+SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) INTO @ja_order FROM t1;
+
+SET @@tmp_memory_table_size=0;
+SELECT GROUP_CONCAT(DISTINCT a) = @gc AS gc_unchanged FROM t1;
+SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) = @gc_order AS gc_order_unchanged FROM t1;
+SELECT JSON_ARRAYAGG(DISTINCT a) = @ja AS ja_unchanged FROM t1;
+SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) = @ja_order AS ja_order_unchanged FROM t1;
+SET @@tmp_memory_table_size=DEFAULT;
+DROP TABLE t1;
+
+--echo #
+--echo # Values wide enough that the filter flushes on nearly every row.
+--echo # Here the ORDER BY case used to return a single value out of 30.
+--echo #
+CREATE TABLE t2 (a VARCHAR(2000)) AS
+  SELECT CONCAT(seq, REPEAT('.', 1990)) AS a FROM seq_1_to_30;
+SELECT COUNT(*) AS rows_in_table, COUNT(DISTINCT a) AS distinct_values FROM t2;
+
+SELECT GROUP_CONCAT(DISTINCT a) INTO @gc FROM t2;
+SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc_order FROM t2;
+
+SET @@tmp_memory_table_size=1000, @@max_heap_table_size=1000;
+SELECT GROUP_CONCAT(DISTINCT a) = @gc AS gc_unchanged FROM t2;
+SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) = @gc_order AS gc_order_unchanged FROM t2;
+SET @@tmp_memory_table_size=DEFAULT, @@max_heap_table_size=DEFAULT;
+DROP TABLE t2;
+
+--echo #
+--echo # ORDER BY does not order the rows that tie on the ordering
+--echo # expression. Which of them comes first is not specified, but it
+--echo # must not depend on the memory available or on the order the rows
+--echo # are read in.
+--echo #
+CREATE TABLE t3 (a BIT(2), b VARCHAR(10), c BIT);
+INSERT INTO t3 VALUES (1, 'a', 0), (0, 'b', 1), (0, 'c', 0), (3, 'd', 1),
+(1, 'e', 1), (3, 'f', 1), (0, 'g', 1);
+SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS at_default FROM t3;
+SET @@tmp_memory_table_size=0;
+SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS at_zero FROM t3;
+SET @@tmp_memory_table_size=DEFAULT;
+
+DELETE FROM t3;
+INSERT INTO t3 VALUES (0, 'c', 0), (0, 'b', 1), (1, 'a', 0), (3, 'd', 1),
+(1, 'e', 1), (3, 'f', 1), (0, 'g', 1);
+SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS reversed_scan_order FROM t3;
+DROP TABLE t3;
+
+--echo #
+--echo # The sort tree can overflow too. Starving it makes repack_tree()
+--echo # cut rows out of the group, which is expected, but the rows that
+--echo # do come back must still be deduplicated and still be in order.
+--echo #
+CREATE TABLE t4 (pk INT AUTO_INCREMENT PRIMARY KEY, a VARCHAR(20));
+INSERT INTO t4 (a) SELECT LPAD(seq, 6, '0') FROM seq_1_to_200;
+INSERT INTO t4 (a) SELECT a FROM t4 ORDER BY pk;
+
+SET @@tmp_memory_table_size=0, @@group_concat_max_len=4000;
+SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc FROM t4;
+SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) INTO @ja FROM t4;
+SET @@tmp_memory_table_size=DEFAULT, @@group_concat_max_len=DEFAULT;
+
+SELECT COUNT(*) = COUNT(DISTINCT val) AS gc_no_duplicates,
+       GROUP_CONCAT(val ORDER BY val) = @gc AS gc_ascending
+FROM (SELECT SUBSTRING_INDEX(SUBSTRING_INDEX(@gc, ',', seq), ',', -1) AS val
+      FROM seq_1_to_500
+      WHERE seq <= 1 + LENGTH(@gc) - LENGTH(REPLACE(@gc, ',', ''))) split;
+
+SELECT JSON_VALID(@ja) AS ja_valid,
+       COUNT(*) = COUNT(DISTINCT val) AS ja_no_duplicates,
+       JSON_ARRAYAGG(val ORDER BY val) = @ja AS ja_ascending
+FROM (SELECT JSON_UNQUOTE(JSON_EXTRACT(@ja, CONCAT('$[', seq - 1, ']'))) AS val
+      FROM seq_1_to_500 WHERE seq <= JSON_LENGTH(@ja)) split;
+DROP TABLE t4;

sql/item_sum.cc

@@ -4296,6 +4296,60 @@ bool Item_func_group_concat::repack_tree(THD *thd)
 }
 
 
+/*
+  Insert one row into the ORDER BY tree, repacking it first if it has
+  grown past the memory we are allowed to use.
+
+  'key' is a temporary table record in the format produced by
+  get_record_pointer(). table->field[0] of that record holds the length
+  the row adds to the result, which is what repack_tree() adds up to
+  decide where to cut.
+
+  @return FALSE  row inserted
+  @return TRUE   out of memory
+*/
+
+bool Item_func_group_concat::insert_to_order_tree(uchar *key)
+{
+  DBUG_ASSERT(tree);
+  THD *thd= table->in_use;
+  /*
+    Repack when GCONCAT_TREE_REPACK_PARTS of the memory we may use is
+    gone. The rest is needed for the new tree that repack_tree()
+    allocates while the old one is still around.
+  */
+  if (tree->allocated >
+      max_tree_size / GCONCAT_TREE_PARTS * GCONCAT_TREE_REPACK_PARTS &&
+      tree->elements_in_tree > 1)
+    if (repack_tree(thd))
+      return TRUE;
+  /* check if there was enough memory to insert the row */
+  return !tree_insert(tree, key, 0, tree->custom_arg);
+}
+
+
+/*
+  Insert one deduplicated row into the ORDER BY tree.
+
+  Callback for Unique::walk(). The walk merges what unique_filter spilled
+  to disk back with what it still holds in memory, so it is the first
+  point at which the whole set of distinct rows is known. Every row it
+  visits belongs in the result.
+
+  @return 0  row inserted
+  @return 1  out of memory, which stops the walk
+*/
+
+int Item_func_group_concat::dump_leaf_key_to_tree(void *key_arg,
+                                                  element_count count
+                                                  __attribute__((unused)),
+                                                  void *item_arg)
+{
+  auto item= static_cast<Item_func_group_concat *>(item_arg);
+  return item->insert_to_order_tree(static_cast<uchar *>(key_arg));
+}
+
+
 bool Item_func_group_concat::add(bool exclude_nulls)
 {
   if (always_null && exclude_nulls)
@@ -4337,7 +4391,15 @@ bool Item_func_group_concat::add(bool exclude_nulls)
   null_value= FALSE;
   bool row_eligible= TRUE;
 
-  if (distinct) 
+  /*
+    Store how much this row adds to the result in the record itself. With
+    DISTINCT the row does not reach the ORDER BY tree until val_str(), and
+    the record kept by unique_filter is all that is left of it by then.
+  */
+  if (tree)
+    table->field[0]->store(row_str_len, FALSE);
+
+  if (distinct)
   {
     /* Filter out duplicate rows. */
     uint count= unique_filter->elements_in_tree();
@@ -4346,26 +4408,16 @@ bool Item_func_group_concat::add(bool exclude_nulls)
       row_eligible= FALSE;
   }
 
-  TREE_ELEMENT *el= 0;                          // Only for safety
-  if (row_eligible && tree)
-  {
-    THD *thd= table->in_use;
-    table->field[0]->store(row_str_len, FALSE);
-    /*
-      Repack when GCONCAT_TREE_REPACK_PARTS of the memory we may use is
-      gone. The rest is needed for the new tree that repack_tree()
-      allocates while the old one is still around.
-    */
-    if (tree->allocated >
-        max_tree_size / GCONCAT_TREE_PARTS * GCONCAT_TREE_REPACK_PARTS &&
-        tree->elements_in_tree > 1)
-      if (repack_tree(thd))
-        return 1;
-    el= tree_insert(tree, get_record_pointer(), 0, tree->custom_arg);
-    /* check if there was enough memory to insert the row */
-    if (!el)
-      return 1;
-  }
+  /*
+    With DISTINCT the ORDER BY tree is not filled here. row_eligible only
+    reflects the part of unique_filter that is currently in memory, so as
+    soon as unique_filter starts flushing to disk it stops telling us
+    whether a row is a duplicate. val_str() fills the tree instead, from
+    the unique_filter walk that merges everything back together.
+  */
+  if (!distinct && row_eligible && tree &&
+      insert_to_order_tree(get_record_pointer()))
+    return 1;
 
   /*
     In case of GROUP_CONCAT with DISTINCT or ORDER BY (or both) don't dump the
@@ -4595,7 +4647,25 @@ String* Item_func_group_concat::val_str(String* str)
 
   if (!result_finalized) // Result yet to be written.
   {
-    if (tree != NULL) // order by
+    if (tree && distinct) // distinct and order by
+    {
+      /*
+        Sort the distinct rows now. add() could not do it, as a row is
+        only known not to be a duplicate once unique_filter has merged
+        everything it flushed to disk, which the walk below does.
+      */
+      if (unique_filter->walk(table, &dump_leaf_key_to_tree, this))
+      {
+        /*
+          Out of memory; mysys has reported it. The tree holds only part
+          of the group, so tell the user that the result was cut instead
+          of returning a short one silently.
+        */
+        result_cut= TRUE;
+      }
+      tree_walk(tree, &dump_leaf_key, this, left_root_right);
+    }
+    else if (tree != NULL) // order by
       tree_walk(tree, &dump_leaf_key, this, left_root_right);
     else if (distinct) // distinct (and no order by).
       unique_filter->walk(table, &dump_leaf_key, this);

sql/item_sum.h

@@ -2199,6 +2199,9 @@ class Item_func_group_concat : public Item_sum_str
   qsort_cmp2 get_comparator_function_for_order_by();
   uchar* get_record_pointer();
   uint get_null_bytes();
+  bool insert_to_order_tree(uchar *key);
+  static int dump_leaf_key_to_tree(void *key_arg, element_count count,
+                                   void *item_arg);
 
 protected:
   Item *shallow_copy(THD *thd) const override