MDEV-40698 ROLLUP query returns incorrect result with empty result set - #5542
MDEV-40698 ROLLUP query returns incorrect result with empty result set#5542jaeheonshim wants to merge 1 commit into
Conversation
|
There's a few other MTR test cases WITH ROLLUP to record too |
e21a68a to
1165d57
Compare
gkodinov
added
the
External Contribution
6ad12b2 to
f83223d
Compare
grooverdan
left a comment
grooverdan
left a comment
There was a problem hiding this comment.
Thanks @jaeheonshim.
Can you rollup, pun intended, these commits into a single final version of a commit message with MDEV header and problem/solution explaination. Include the explain as to why InnoDB is different.
| DROP TABLE t0, t1; | ||
|
|
||
| --echo # | ||
| --echo # End of 10.11 tests |
There was a problem hiding this comment.
just single line on End of x tests.
Can the tests be appended to mysql-test/main/olap.test instead of a new file.
There was a problem hiding this comment.
I initially had it that way, but the olap tests don't include innodb and I wasn't sure about including it for the whole file just for these tests
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution. This is a preliminary review.
Daniel already marked the relevant points that I'd otherwise cover during the preliminary review. Please work with him to address these.
Since I have nothing more to add I'm marking it as LGTM.
There was a problem hiding this comment.
the patch conflicts with current IN to EXISTS mechanisms. Here is an example
CREATE TABLE t (a INT);
SELECT 1 IN (SELECT a FROM t WHERE 1=0 GROUP BY a WITH ROLLUP);
1 in NULL should be a NULL, but after the patch, we get 1.
and before the patch, we get an also incorrect 0
thd::limit_found_rows needs to be reset when sending out one of these new empty result sets.
| *empty_set_send_rollup_total= | ||
| end_of_records && !first_record && rollup.state != ROLLUP::STATE_NONE; | ||
| return (end_of_records && !first_record && !group && | ||
| !group_optimized_away) || |
There was a problem hiding this comment.
this is a new condition. Do we need this?
There was a problem hiding this comment.
It's mainly for code cleanliness. Previously the need_empty_set_row condition was present directly in the if statement in end_send_group/end_write_group. But I figured with the extra condition for empty_set_rollup_total, which needs to be referenced again later, the code would start having many duplicated expressions.
There was a problem hiding this comment.
sorry, i meant group_optimized_away. It's likely not an issue, i was curious.
There was a problem hiding this comment.
ohh i see. actually I am curious too because group_optimized_away was included in the conditional in end_send_group but not end_write_group. But I believe it should be included in both to be consistent with send_row_on_empty_set
f83223d to
246d69d
Compare
ROLLUP is defined as the UNION of grouping by every prefix of fields in
the original GROUP BY list. A ROLLUP query with an empty result set
returned zero rows when it should return a summary NULL, since grouping
by the empty prefix returns a single summary row.
The empty row case is handled in two separate locations. First, if the
optimizer is able to determine that no rows will be produced, e.g. due
to the table being empty or the WHERE condition resolving to false,
JOIN::send_row_on_empty_set is used to determine whether or not to send
an empty result row. Therefore, send_row_on_empty_set is modified to
include select_lex->olap == ROLLUP_TYPE.
Second, it may be the case that the absence of rows is not confirmed
until the execution phase. For instance when the WHERE condition is not
constant, or in the case of InnoDB where an empty table is not detected
during optimization. This is handled in both end_send_group and
end_write_group by this expression
join->first_record ||
(end_of_records && !join->group && !join->group_optimized_away)
The condition is extracted into need_empty_set_row and a second variable
empty_set_send_rollup_total is recorded to prevent running the default
rollup_send_data/rollup_write_data on the empty row case. This is
because the null summary row is already handled by send_data_with_check.
246d69d to
3b89ded
Compare
|
Fixed the IN -> EXISTS mechanism by patching opt_subselect.cc |
|
FYI: According to our development cycle we work on bugs In the following periods 15 Mar-30 Apr, 15 Jun-30 Jul, 15 Sep-30 Oct and 15 Dec-31 Jan. So, please, expect to get a review somewhere between these two dates and the goal is to have your PR merged before the second date |
4 participants
Fixes MDEV-40698