Hi Varun, I only have input about comments/variable names. Please find it below. On Tue, Mar 24, 2020 at 10:14:38AM +0530, Varun wrote:
revision-id: 4f528bb514c607d848b30119cea135e37e0d9c69 (mariadb-10.5.0-425-g4f528bb514c) parent(s): 1cd8e8213b6019794ca7f01e8276cfd1ba6408cf author: Varun Gupta committer: Varun Gupta timestamp: 2020-03-24 10:13:28 +0530 message:
MDEV-22011: DISTINCT with JSON_ARRAYAGG gives wrong results
For JSON_ARRAYAGG with DISTINCT also store the null bytes for the record inside the Unique object
--- mysql-test/main/func_json.result | 4 +- sql/item_jsonfunc.cc | 36 ++++++++++++-- sql/item_jsonfunc.h | 9 ++-- sql/item_sum.cc | 101 ++++++++++++++++++++++++++++++++++----- sql/item_sum.h | 13 +++-- 5 files changed, 133 insertions(+), 30 deletions(-)
diff --git a/sql/item_jsonfunc.cc b/sql/item_jsonfunc.cc index a3b5d3b7fac..ed496df8169 100644 --- a/sql/item_jsonfunc.cc +++ b/sql/item_jsonfunc.cc @@ -1452,6 +1452,31 @@ static int append_json_value(String *str, Item *item, String *tmp_val) }
+bool append_json_value(String *str, String *res, Item *item, bool null_value)
There is another append_json_value right above this one. That function has no comments but let's not follow that bad pattern. - Please add a function comment describing this function: @brief should describe this function @detail should describe the difference from the one above. It is very counter-intuitive that the parameter named "res" does not hold the result of this function. Please rename it (to value?) (can it be changed to be const String* ?)
+{ + if (null_value) + return str->append("null", 4); + + if (item->type_handler()->is_bool_type()) + { + return ((*res)[0] == '1') ? + str->append("true", 4) : + str->append("false", 5); + } + + if (item->is_json_type()) + return str->append(res->ptr(), res->length()); + + if (item->result_type() == STRING_RESULT) + { + return str->append("\"", 1) || + st_append_escaped(str, res) || + str->append("\"", 1); + } + return st_append_escaped(str, res);
This made me wonder why something is escaped if it is not enclosed in quotes, but now I see this is just copied from above.
+} + + static int append_json_keyname(String *str, Item *item, String *tmp_val) { String *sv= item->val_str(tmp_val);
diff --git a/sql/item_sum.cc b/sql/item_sum.cc index 5c732095573..a04dbdc19e2 100644 --- a/sql/item_sum.cc +++ b/sql/item_sum.cc @@ -3552,6 +3552,54 @@ int group_concat_key_cmp_with_distinct(void* arg, const void* key1, }
+int group_concat_key_cmp_with_distinct_with_nulls(void* arg, + const void* key1_arg, + const void* key2_arg) +{ + Item_func_group_concat *item_func= (Item_func_group_concat*)arg; + + uchar *key1= (uchar*)key1_arg + item_func->table->s->null_bytes; + uchar *key2= (uchar*)key2_arg + item_func->table->s->null_bytes; + + for (uint i= 0; i < item_func->arg_count_field; i++)
This loop is actually not needed, right? JSON_ARRAYAGG only accepts one argument. Please add a note.
+ { + Item *item= item_func->args[i]; + /* + If 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 + */ + 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 -1; + + if (field->is_null_in_record((uchar*)key2_arg)) + return 1; + + uint offset= (field->offset(field->table->record[0]) - + field->table->s->null_bytes); + int res= field->cmp(key1 + offset, key2 + offset); + if (res) + return res; + } + return 0; +} + + /** function of sort for syntax: GROUP_CONCAT(expr,... ORDER BY col,... ) */ @@ -3650,9 +3699,11 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)), result->append(*item->separator);
+ bool is_null= false; for (; arg < arg_end; arg++) { String *res; + is_null= false; (1) /* We have to use get_tmp_table_field() instead of real_item()->get_tmp_table_field() because we want the field in @@ -3661,7 +3712,11 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)), because it contains both order and arg list fields. */ if ((*arg)->const_item()) + { res= (*arg)->val_str(&tmp); + if ((*arg)->null_value) + is_null= TRUE; (2) Please either use true/false (preferred) or TRUE / FALSE.
+ } else { Field *field= (*arg)->get_tmp_table_field(); ...
diff --git a/sql/item_sum.h b/sql/item_sum.h index b10d9f5974d..908dbe3cce9 100644 --- a/sql/item_sum.h +++ b/sql/item_sum.h @@ -1897,14 +1899,19 @@ class Item_func_group_concat : public Item_sum */ Item_func_group_concat *original;
+ bool exclude_nulls; + /* Used by Item_func_group_concat and Item_func_json_arrayagg. The latter needs null values but the former doesn't. */ Is this comment relevant here? I think it's better to remove this and instead add a comment for the exclude_nulls above.
- bool add(bool exclude_nulls); + bool add();
friend int group_concat_key_cmp_with_distinct(void* arg, const void* key1, const void* key2); + friend int group_concat_key_cmp_with_distinct_with_nulls(void* arg, + const void* key1, + const void* key2); friend int group_concat_key_cmp_with_order(void* arg, const void* key1, const void* key2); friend int dump_leaf_key(void* key_arg,
BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog