Optimizer: Fix null filtering logic for IN list #53370
Conversation
ti-chi-bot
bot
added
release-note-none
do-not-merge/needs-triage-completed
sig/planner
|
Hi @ghazalfamilyusa. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #53370 +/- ##
================================================
+ Coverage 72.6008% 73.0911% +0.4902%
================================================
Files 1505 1523 +18
Lines 429827 439569 +9742
================================================
+ Hits 312058 321286 +9228
+ Misses 98576 98362 -214
- Partials 19193 19921 +728
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ghazalfamilyusa
force-pushed
the
nullability_bug
branch
from
6f08f72
to
59e3d2f
Compare
ti-chi-bot
bot
added
size/XL
|
/ok-to-test |
ghazalfamilyusa
force-pushed
the
nullability_bug
branch
from
59e3d2f
to
a3479df
Compare
|
I got the idea! |
pkg/planner/util/null_misc.go
Outdated
| return specialNullRejectedCase1(ctx, schema, expr) // only 1 case now | ||
| // isNullRejectedInList checks null filter for IN list using OR logic. | ||
| // Reason is that null filtering through evaluation by isNullRejectedSimpleExpr | ||
| // has problems with IN list. |
There was a problem hiding this comment.
Better to add a case here to explain why we have to treat IN-LIST as OR here. The rest LGTM!
There was a problem hiding this comment.
So if the in-list contains a column from the non-inner side, then it can never pass the NF check.
There was a problem hiding this comment.
The explanation for this case is shown above and in the issue. I guess you guys think it is better to add it in the code. Added that to the comment.
pkg/planner/util/null_misc.go
Outdated
|
|
||
| // isNullRejectedSimpleExpr check whether a condition is null-rejected | ||
| // A condition would be null-rejected in one of following cases: | ||
| // If it is a predicate containing a reference to an inner table that evaluates to UNKNOWN or FALSE |
There was a problem hiding this comment.
i think the comment should be refined as well, "inner table" has no ideas with
There was a problem hiding this comment.
added explanation to inner table as "null producing side"
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, qw4990, winoros The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ghazalfamilyusa
force-pushed
the
nullability_bug
branch
from
0084e77
to
03dc40b
Compare
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
What problem does this PR solve?
Issue Number: close #49476
Problem Summary:
Incorrect outer to inner join conversion for some cases of IN list incorrectly treated as null filtering.
An example in #49476
CREATE TABLE t0(c0 INT);
CREATE TABLE t1(c0 BOOLEAN);
INSERT INTO t0 (c0) VALUES (0);
SELECT * FROM t1 NATURAL RIGHT JOIN t0; -- 0
SELECT (false IN (t1.c0, t0.c0)) FROM t1 NATURAL RIGHT JOIN t0; -- 1
SELECT * FROM t1 NATURAL RIGHT JOIN t0 WHERE (false IN (t1.c0, t0.c0));
-- Expected: 0
-- Actual: Empty set
The problem is in the constant folding logic when it is used to evaluate null rejection logic.
The logic is risky and just fills in constant 1 for non-inner side. In the example above the IN list becomes
false in (null,1) which is false (null filter is true).
What changed and how does it work?
Fix done at the top level code for null filtering logic used in outer to inner. The fix use the OR logic instead of IN list.
For example, alse IN (t1.c0, t0.c0)) is checked for nullability using (false = t1.c0) or (false = t0.c0) which is not null filter.
Also, this PR consolidates the null rejection code from outer to inner code and the general utility code. This includes cleans up of previous special cases.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.