MDEV-22840: JSON_ARRAYAGG gives wrong results with NULL values and ORDER by clause

The problem here is similar to the case with DISTINCT, the tree used for ORDER BY
needs to also hold the null bytes of the record. This was not done for GROUP_CONCAT
as NULLS are rejected by GROUP_CONCAT.

Also introduced a comparator function for the order by tree to handle null
values with JSON_ARRAYAGG.

mysql-test/main/func_json.result

@@ -1284,5 +1284,30 @@ JSON_ARRAYAGG(DISTINCT a)
 [null,"1","2","3"]
 DROP TABLE t1;
 #
+# MDEV-22840: JSON_ARRAYAGG gives wrong results with NULL values and ORDER by clause
+#
+CREATE TABLE t1(a VARCHAR(255));
+INSERT INTO t1 VALUES ('red'),('blue');
+SELECT JSON_ARRAYAGG(a) FROM t1;
+JSON_ARRAYAGG(a)
+["red","blue"]
+SELECT JSON_ARRAYAGG(a ORDER BY a DESC) FROM t1;
+JSON_ARRAYAGG(a ORDER BY a DESC)
+["red","blue"]
+SELECT JSON_ARRAYAGG(a ORDER BY a ASC) FROM t1;
+JSON_ARRAYAGG(a ORDER BY a ASC)
+["blue","red"]
+INSERT INTO t1 VALUES (NULL);
+SELECT JSON_ARRAYAGG(a) FROM t1;
+JSON_ARRAYAGG(a)
+["red","blue",null]
+SELECT JSON_ARRAYAGG(a ORDER BY a DESC) FROM t1;
+JSON_ARRAYAGG(a ORDER BY a DESC)
+["red","blue",null]
+SELECT JSON_ARRAYAGG(a ORDER BY a ASC) FROM t1;
+JSON_ARRAYAGG(a ORDER BY a ASC)
+[null,"blue","red"]
+DROP TABLE t1;
+#
 # End of 10.5 tests
 #

mysql-test/main/func_json.test

@@ -794,6 +794,24 @@ SELECT JSON_ARRAYAGG(DISTINCT a) FROM t1;
 
 DROP TABLE t1;
 
+--echo #
+--echo # MDEV-22840: JSON_ARRAYAGG gives wrong results with NULL values and ORDER by clause
+--echo #
+
+CREATE TABLE t1(a VARCHAR(255));
+INSERT INTO t1 VALUES ('red'),('blue');
+
+SELECT JSON_ARRAYAGG(a) FROM t1;
+SELECT JSON_ARRAYAGG(a ORDER BY a DESC) FROM t1;
+SELECT JSON_ARRAYAGG(a ORDER BY a ASC) FROM t1;
+
+INSERT INTO t1 VALUES (NULL);
+
+SELECT JSON_ARRAYAGG(a) FROM t1;
+SELECT JSON_ARRAYAGG(a ORDER BY a DESC) FROM t1;
+SELECT JSON_ARRAYAGG(a ORDER BY a ASC) FROM t1;
+DROP TABLE t1;
+
 --echo #
 --echo # End of 10.5 tests
 --echo #

sql/item_sum.cc

@@ -3667,6 +3667,72 @@ int group_concat_key_cmp_with_order(void* arg, const void* key1,
 }
 
 
+/*
+  @brief
+    Comparator function for ORDER BY clause taking into account NULL values.
+
+  @note
+    Used for JSON_ARRAYAGG function
+*/
+
+int group_concat_key_cmp_with_order_with_nulls(void *arg, const void *key1_arg,
+                                               const void *key2_arg)
+{
+  Item_func_group_concat* grp_item= (Item_func_group_concat*) arg;
+  ORDER **order_item, **end;
+
+  uchar *key1= (uchar*)key1_arg + grp_item->table->s->null_bytes;
+  uchar *key2= (uchar*)key2_arg + grp_item->table->s->null_bytes;
+
+  for (order_item= grp_item->order, end=order_item+ grp_item->arg_count_order;
+       order_item < end;
+       order_item++)
+  {
+    Item *item= *(*order_item)->item;
+    /*
+      If field_item is a const item then either get_tmp_table_field returns 0
+      or it is an item over a const table.
+    */
+    if (item->const_item())
+      continue;
+    /*
+      We have to use get_tmp_table_field() instead of
+      real_item()->get_tmp_table_field() because we want the field in
+      the temporary table, not the original field
+
+      Note that for the case of ROLLUP, field may point to another table
+      tham grp_item->table. This is however ok as the table definitions are
+      the same.
+    */
+    Field *field= item->get_tmp_table_field();
+    if (!field)
+      continue;
+
+    if (field->is_null_in_record((uchar*)key1_arg) &&
+        field->is_null_in_record((uchar*)key2_arg))
+      continue;
+
+    if (field->is_null_in_record((uchar*)key1_arg))
+      return ((*order_item)->direction == ORDER::ORDER_ASC) ?  -1 : 1;
+
+    if (field->is_null_in_record((uchar*)key2_arg))
+      return ((*order_item)->direction == ORDER::ORDER_ASC) ?  1 :  -1;
+
+    uint offset= (field->offset(field->table->record[0]) -
+                  field->table->s->null_bytes);
+    int res= field->cmp((uchar*)key1 + offset, (uchar*)key2 + offset);
+    if (res)
+      return ((*order_item)->direction == ORDER::ORDER_ASC) ? res : -res;
+  }
+  /*
+    We can't return 0 because in that case the tree class would remove this
+    item as double value. This would cause problems for case-changes and
+    if the returned values are not the same we do the sort on.
+  */
+  return 1;
+}
+
+
 /**
   Append data from current leaf to item->result.
 */
@@ -4015,7 +4081,7 @@ bool Item_func_group_concat::repack_tree(THD *thd)
 
   init_tree(&st.tree, (size_t) MY_MIN(thd->variables.max_heap_table_size,
                                       thd->variables.sortbuff_size/16), 0,
-            size, group_concat_key_cmp_with_order, NULL,
+            size, get_comparator_function_for_order_by(), NULL,
             (void*) this, MYF(MY_THREAD_SPECIFIC));
   DBUG_ASSERT(tree->size_of_element == st.tree.size_of_element);
   st.table= table;
@@ -4101,8 +4167,7 @@ bool Item_func_group_concat::add(bool exclude_nulls)
         && tree->elements_in_tree > 1)
       if (repack_tree(thd))
         return 1;
-    el= tree_insert(tree, table->record[0] + table->s->null_bytes, 0,
+    el= tree_insert(tree, get_record_pointer(), 0, tree->custom_arg);
-                    tree->custom_arg);
     /* check if there was enough memory to insert the row */
     if (!el)
       return 1;
@@ -4306,8 +4371,8 @@ bool Item_func_group_concat::setup(THD *thd)
     */
     init_tree(tree, (size_t)MY_MIN(thd->variables.max_heap_table_size,
                                    thd->variables.sortbuff_size/16), 0,
-              tree_key_length,
+              tree_key_length + get_null_bytes(),
-              group_concat_key_cmp_with_order, NULL, (void*) this,
+              get_comparator_function_for_order_by(), NULL, (void*) this,
               MYF(MY_THREAD_SPECIFIC));
     tree_len= 0;
   }
@@ -4384,6 +4449,19 @@ qsort_cmp2 Item_func_group_concat::get_comparator_function_for_distinct()
 }
 
 
+/*
+  @brief
+    Get the comparator function for ORDER BY clause
+*/
+
+qsort_cmp2 Item_func_group_concat::get_comparator_function_for_order_by()
+{
+  return skip_nulls() ?
+         group_concat_key_cmp_with_order :
+         group_concat_key_cmp_with_order_with_nulls;
+}
+
+
 /*
 
   @brief

sql/item_sum.h

@@ -1853,6 +1853,8 @@ int group_concat_key_cmp_with_distinct_with_nulls(void* arg, const void* key1,
                                                   const void* key2);
 int group_concat_key_cmp_with_order(void* arg, const void* key1,
                                     const void* key2);
+int group_concat_key_cmp_with_order_with_nulls(void *arg, const void *key1,
+                                               const void *key2);
 int dump_leaf_key(void* key_arg,
                   element_count count __attribute__((unused)),
                   void* item_arg);
@@ -1920,6 +1922,8 @@ protected:
                                                            const void* key2);
   friend int group_concat_key_cmp_with_order(void* arg, const void* key1,
 					     const void* key2);
+  friend int group_concat_key_cmp_with_order_with_nulls(void *arg,
+                                       const void *key1, const void *key2);
   friend int dump_leaf_key(void* key_arg,
                            element_count count __attribute__((unused)),
 			   void* item_arg);
@@ -2010,6 +2014,7 @@ public:
   Item *get_copy(THD *thd)
   { return get_item_copy<Item_func_group_concat>(thd, this); }
   qsort_cmp2 get_comparator_function_for_distinct();
+  qsort_cmp2 get_comparator_function_for_order_by();
   uchar* get_record_pointer();
   uint get_null_bytes();