Hello Raghu, This is a review for your patch for MDEV-36851

· MDEV-36581: Fix nullability logic of COALESCE

The MDEV number in the comment is incorrect. Should be MDEV-36851 Please next time make sure to copy the MDEV number together with its title from Jira, to avoid typos.

COALESCE currently uses a generic logic- "Result of a function is nullable if any of the arguments is nullable", which is wrong.

This patch fixes the logic behind the nullability of COALESCE to make the result NULL, only if all of the arguments are NULL.

Tests included in `mysql-test/main/func_hybrid_type.test`

The general idea of the patch is correct. I have small suggestions for the implementation. <cut>

--- a/sql/item_cmpfunc.h +++ b/sql/item_cmpfunc.h @@ -1148,6 +1148,14 @@ class Item_func_interval :public Item_long_func

class Item_func_coalesce :public Item_func_case_expression { +protected: + bool all_args_maybe_null= true; + inline void set_nullability_with(const Item *item) override + { + if (all_args_maybe_null && !(bool) (item->base_flags & item_base_t::MAYBE_NULL)) + all_args_maybe_null= false; + base_flags|= item->base_flags & item_base_t::MAYBE_NULL > + }

I don't think we need a new member and a new virtual method. This is too complex to fix this problem. Please (instead of trying to build-in the new code inside fix_fields()) add a loop into fix_length_and_dec() searching for a non-NULL argument and removing the MAYBE_NULL flag if such argument is found. I also suggest additional tests: without view and without derived tables. Just using regular tables. Thanks.

public: Item_func_coalesce(THD *thd, Item *a, Item *b): Item_func_case_expression(thd, a, b) {} @@ -1162,6 +1170,12 @@ class Item_func_coalesce :public Item_func_case_expression bool native_op(THD *thd, Native *to) override; bool fix_length_and_dec(THD *thd) override { + /* + Result of COALESCE is nullable iff all arguments + are NULL, otherwise NOT NULL. + */ + if (!all_args_maybe_null) + base_flags &= ~item_base_t::MAYBE_NULL; if (aggregate_for_result(func_name_cstring(), args, arg_count, true)) return TRUE; fix_attributes(args, arg_count); <cut>