GROUP_CONCAT(expr ORDER BY col) mis-sorts where col is NULL

Description

The ORDER BY key stored in the tree does not include the record's NULL bytes, and group_concat_key_cmp_with_order() never looks at the NULL bits. A NULL ORDER BY value is therefore compared using whatever data bytes the previously processed row happened to leave behind in the table record.

Steps to reproduce

MariaDB [test]> CREATE TABLE t (a INT, b INT, c INT);

Query OK, 0 rows affected (0.063 sec)

MariaDB [test]> INSERT INTO t VALUES (0, 1, 10), (NULL, 2, 20), (0, 3, 30);

Query OK, 3 rows affected (0.005 sec)

Records: 3  Duplicates: 0  Warnings: 0

MariaDB [test]> SELECT * FROM t ORDER BY a, b;

+------+------+------+

| a    | b    | c    |

+------+------+------+

| NULL |    2 |   20 |

|    0 |    1 |   10 |

|    0 |    3 |   30 |

+------+------+------+

3 rows in set (0.000 sec)

MariaDB [test]> SELECT GROUP_CONCAT(c ORDER BY a, b) FROM t;

+-------------------------------+

| GROUP_CONCAT(c ORDER BY a, b) |

+-------------------------------+

| 10,20,30                      |

+-------------------------------+

1 row in set (0.000 sec)

The last result should be 20,10,30, matching the row order shown by SELECT * FROM t ORDER BY a, b: with ascending ORDER BY, NULL sorts first, so row (NULL, 2, 20) comes before (0, 1, 10).

Root cause

The original code reads:

  /*

     Need sorting or uniqueness: init tree and choose a function to sort.

     Don't reserve space for NULLs: if any of gconcat arguments is NULL,

     the row is not added to the result.

  */

  uint tree_key_length= table->s->reclength - table->s->null_bytes;

The comment is inaccurate. In GROUP_CONCAT(c ORDER BY a, b), a row is skipped only when c is NULL; a and b may also be NULL, and such rows are still added to the result. The red-black tree used for sorting therefore still needs to handle NULL values.

Notes

• Not a regression. The sort key layout was introduced by MySQL commit 6fc7c0742e9 ("Cleanup of Item_func_group_concat", 2005-03-18), whose commit message says "don't store NULLs in the tree"; the comment quoted above was added by MySQL commit cdeecc51547 (Bug#32798, 2007-12-14). It came into MariaDB with the mysql-5.5.32 merge (005c7e542145, 2013-07-16).
• I have a patch with a regression test that fails on the unpatched server and passes with the fix, and will submit it as a pull request.