Hi Rex,
Please find below input for the commit.
(I've actually committed an alternative fix now, but before doing that I wrote
all the trivial comments below so I'm sending these anyway in order to show
what is expected of a patch. I'm also including the rationale for creating
an alternative patch)
> commit 75af3195482d3da277825663d1150c0dfc55420a
> Author: Rex <rex.johns...@mariadb.com>
> Date: Fri Dec 23 05:28:59 2022 +1200
>
> MDEV-28602 Wrong result with outer join, merged derived table and view
>
> const item reference being mishandled in outer to inner join on
> filling in null values.
>
First, fixVersion. It is currently set to 10.11, but affectsVersion starts from
10.3
I think the bug and the fix are applicable for all versions.
Second: commit comment. This is not descriptive enough:
> const item reference being mishandled in outer to inner join on
> filling in null values.
Something like this would be better:
<commit-comment>
A LEFT JOIN with a constant as a column of the inner table produced wrong
query
result if the optimizer had to write the inner table column into a temp table:
SELECT ...
FROM (SELECT /*non-mergeable select*/
FROM t1 LEFT JOIN (SELECT 'Y' as Val) t2 ON ...)
Fixed this by:
1. Making Item_ref::save_in_field() call check_null_ref() to see if the
referred
table has a NULL-complemented row.
2. In order to do #1, moved null_ref_table() and check_null_ref() from
Item_direct_view_ref to Item_ref.
</commit-comment>
Note: it starts with a siccint problem description, which is followed by a
description of the fix.
> diff --git a/sql/sql_join_cache.cc b/sql/sql_join_cache.cc
> index 620c52a3f40..65bedc3d337 100644
> --- a/sql/sql_join_cache.cc
> +++ b/sql/sql_join_cache.cc
> @@ -2622,7 +2622,7 @@ enum_nested_loop_state
> JOIN_CACHE::join_null_complements(bool skip_last)
> get_record();
> /* The outer row is complemented by nulls for each inner table */
> restore_record(join_tab->table, s->default_values);
> - mark_as_null_row(join_tab->table);
> + mark_as_null_row(join_tab->table);
In the future please make sure patches do not contain unrelated meaningless
changes like this one.
> rc= generate_full_extensions(get_curr_rec());
> if (rc != NESTED_LOOP_OK && rc != NESTED_LOOP_NO_MORE_ROWS)
> goto finish;
> diff --git a/mysql-test/main/merge.test b/mysql-test/main/merge.test
> index 0485f3ed1c3..05b05287e1b 100644
> --- a/mysql-test/main/merge.test
> +++ b/mysql-test/main/merge.test
Why is the patch in the merge.test ?
merge.test at the top says clearly "Test of MERGE TABLES".
That is, this is a test for tables with ENGINE=MERGE, a feature not related to
this bug.
The right test files for this bug would be main/join_outer.test and
main/derived.test,
I would prefer the first one.
> @@ -2886,3 +2886,50 @@ drop table tm, t;
> --echo #
> --echo # End of 10.8 tests
> --echo #
> +
> +--echo #
> +--echo # MDEV-28602 Wrong result with outer join, merged derived table and
> view
> +--echo #
> +
> +drop table if exists t1, t2;
^^^ This is not needed as there is a DROP TABLE right above
> +create table t1 (
> + Election int(10) unsigned NOT NULL
> +);
> +
> +insert into t1 (Election) values (1);
> +
+create table t2 (
+ VoteID int(10),
+ ElectionID int(10),
+ UserID int(10)
+);
+
+insert into t2 (ElectionID, UserID) values (2, 30), (3, 30);
+# INSERT INTO t2 (ElectionID, UserID) VALUES (1, 30);
^^ No need to have commented-out lines in the commit.
+drop view if exists v1;
^^^ same as above: this is not needed.
+create view v1 as select * from t1
+ left join ( select 'Y' AS Voted, ElectionID from t2 ) AS T
+ on T.ElectionID = t1.Election
+limit 9;
+# limit X causes merge algorithm select as opposed to temp table
+select * from v1;
...
> diff --git a/sql/item.h b/sql/item.h
> index 2d598546b91..8684477b558 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -5531,18 +5531,42 @@ class Item_sp
>
> class Item_ref :public Item_ident
> {
> +public:
> +
> +#define NO_NULL_TABLE (reinterpret_cast<TABLE *>(0x1))
> +
> + void set_null_ref_table()
> + {
> + null_ref_table= NO_NULL_TABLE;
> + }
> +
> + bool check_null_ref()
This works, but looks like a change that is
1. Too big
2. Looks like a partial implementation
I'm looking at methods of Item_direct_view_ref:
- save_val()
- save_org_in_field()
- save_in_result_field()
- val_real() and other val_XXX()
and they all follow the same pattern:
if (check_null_ref())
produce SQL NULL;
else
Call Item_direct_ref::same_method();
This suggests we could add Item_direct_view_ref::save_in_field() and use the
same approach.
Conversely, if we move check_null_ref() to be in Item_ref, we will end up with:
- check_null_ref() is in Item_ref
- methods that make use of it are in Item_direct_view_ref()
- methods of Item_ref() do not make check_null_ref() check, except for
save_in_field()
which does it.
I think the former looks more logical than the latter. I've made the patch for
it
and I'll ask Sanja (the Item_ref/views expert) to check it.
BR
Sergei
--
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
