Hi Oleksandr,
Thanks for addressing the previous review comments. I only have some
minor comments for this round - please see below:
> From 083d2ea18210948266474278b3263063020da5f8 Mon Sep 17 00:00:00 2001
> From: Oleksandr Byelkin <sa...@mariadb.com>
> Date: Mon, 23 Jun 2025 18:02:12 +0200
> Subject: [PATCH] MDEV-36866 postreview.
> [... 167 lines elided]
> +--echo # optimizer do not optimize out subselects but it guaranty the same
> in future
With grammar and typo fix this should be
--echo # The optimizer does not optimize out subselects but it guarantess the
same in future
But I don't understand the comment here. What is "optimize out
subselects"? What "the same"? The output of the explain extended is
quite a mouthful and I'm not sure what I'm supposed to look at in
relation to this comment. I tried the equivalent "left join" in case the
comparison explains the comment, but it outputs the same:
explain extended
select * from t1 left join t2 on t1.a = t2.a where
isnull(t2.a in (select a from t1));
> +explain extended
> +select * from t1,t2 where t1.a = t2.a (+) and
> + isnull(t2.a in (select a from t1));
> +
> +explain extended
> +select * from t1,t2 where t1.a = t2.a (+) and
> + isnull(t2.a in (1, 2, 3));
> +
> +explain extended
> +select * from t1,t2 where t1.a = t2.a (+) and
> + isnull(t1.a in (1, 2, 3));
> +
> +explain extended
> +select * from t1,t2 where t1.a = t2.a (+) and
> + isnull(isnull(t2.a));
> +
> +explain extended
> +select * from t1,t2 where t1.a = t2.a (+) and
> + isnull(field(t2.a, 2, 23));
> +
> +explain extended
> +select * from t1,t2 where t1.a = t2.a (+) and
> + isnull(benchmark(10, t2.a));
> +
> +
> +drop table t1, t2;
> +
> --echo # End of 12.1 tests
> diff --git a/sql/item.h b/sql/item.h
> index 5b4ca045cc9..e0717ab23b3 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -2292,7 +2292,13 @@ class Item :public Value_source,
> return 0;
> }
> virtual bool ora_join_processor(void *arg) { return 0; }
> - virtual bool recalc_maybe_null_processor(void *arg) { return 0; }
> + /*
> + This marks the item as nullable. Note that if we'd want a method that
> + marks the item as not nullable (maybe_null=false) we'd need to process
> + carefully functions (e.g. json*) that can always return null even with
> + non-null arguments
> + */
> + virtual bool add_maybe_null_after_ora_join_processor(void *arg) { return
> 0; }
> virtual bool remove_ora_join_processor(void *arg)
> {
> with_flags&= ~item_with_t::ORA_JOIN;
> @@ -2906,7 +2912,7 @@ class Item_args
> }
> return false;
> }
> - bool arg_check_maybe_null()
> + bool is_any_arg_imaybe_null()
I assume it's intentional here that i and maybe are concatenated in the
method name here.
> {
> for (uint i= 0; i < arg_count; i++)
> {
> @@ -2915,6 +2921,15 @@ class Item_args
> }
> return false;
> }
> + bool is_all_arg_imaybe_null()
> + {
> + for (uint i= 0; i < arg_count; i++)
> + {
> + if (args[i]->maybe_null())
> + return false;
> + }
> + return true;
> + }
> bool transform_args(THD *thd, Item_transformer transformer, uchar *arg);
> void propagate_equal_fields(THD *, const Item::Context &, COND_EQUAL *);
> bool excl_dep_on_table(table_map tab_map)
> @@ -3955,8 +3970,10 @@ class Item_field :public Item_ident,
> return 0;
> }
> bool ora_join_processor(void *arg) override;
> - bool recalc_maybe_null_processor(void *arg) override
> + bool add_maybe_null_after_ora_join_processor(void *arg) override
> {
> + // we only add maybe_null flag
> + DBUG_ASSERT(!maybe_null() || field->maybe_null());
It took me a while to understand why this assert is supposed to hold.
I think a more explanatory comment would be (use past tense):
// we only added maybe_null flags (to tables)
> [... 42 lines elided]
> diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h
> index c65d5253c34..ff21264c751 100644
> --- a/sql/item_cmpfunc.h
> +++ b/sql/item_cmpfunc.h
> @@ -279,6 +279,9 @@ class Item_func_truth : public Item_bool_func
> {
> return negated_item(thd);
> }
> + // block standard processor for never null
> + bool add_maybe_null_after_ora_join_processor(void *arg) override
> + { return 0; }
What is "block standard processor"? I get "standard", but what is
"block"?
> [... 21 lines elided]
> @@ -1216,6 +1225,12 @@ class Item_func_coalesce :public
> Item_func_case_expression
> fix_attributes(args, arg_count);
> return FALSE;
> }
> + bool add_maybe_null_after_ora_join_processor(void *arg) override
> + {
> + if (!maybe_null())
> + set_maybe_null(is_all_arg_imaybe_null());
> + return 0;
> + }
I thought we are not supposed to mark "not nullable" in these methods.
If that is the case, shouldn't this be
if (!maybe_null() && is_all_arg_imaybe_null())
set_maybe_null();
? Same for Item_func_ifnull below.
> [... 91 lines elided]
> diff --git a/sql/item_strfunc.h b/sql/item_strfunc.h
> index 7bf5f0c9c0f..0223b266b34 100644
> --- a/sql/item_strfunc.h
> +++ b/sql/item_strfunc.h
> @@ -210,6 +210,14 @@ class Item_func_to_base64 :public
> Item_str_ascii_checksum_func
> }
> Item *do_get_copy(THD *thd) const override
> { return get_item_copy<Item_func_to_base64>(thd, this); }
> + bool add_maybe_null_after_ora_join_processor(void *arg) override
> + {
> + if (!maybe_null() && args[0]->maybe_null())
> + {
> + set_maybe_null();
> + }
> + return 0;
> + }
> };
Looks like this overriding implementation is not needed because
Item_func_to_base64 has only one argument and the method can fall back
to that of Item_func_or_sum.
> [... 128 lines elided]
Best,
Yuchen
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
