Fix correctness bug in EffectivePredicateExtractor #23674
Conversation
aaneja
force-pushed
the
bugfixEqualityInference
branch
from
92315e0
to
dd3ff3a
Compare
to not pull up expressions only referring to constants
aaneja
force-pushed
the
bugfixEqualityInference
branch
from
dd3ff3a
to
c88ea85
Compare
aaneja
changed the title
aaneja
requested review from
jaystarshot,
feilong-liu,
elharo,
ClarenceThreepwood and
a team
as code owners
|
Should we add a test from #23660? |
| effectiveConjuncts.add(rewritten); | ||
| } | ||
| // If equality inference has reduced the predicate to an expression referring to only constants, it does not make sense to pull this predicate up |
There was a problem hiding this comment.
Bit confused here, as to why do we have the condition to skip if rewritten has variable references? What if the variable reference is pointing to a constant in some project below?
There was a problem hiding this comment.
I guess if it is below, then correctness should already be correct.
There was a problem hiding this comment.
If rewritten has variable references that are in(variables) we do not skip them and pull them up (by adding them to effectiveConjuncts. The problem here is that EqaulityInference is designed to rewrite expressions to just constants (see test), even though, technically this expression does not refer to any variables
However, if the rewritten expression has no variable references, le.g mod(10,5)=1 - this predicate should not be pulled up, to say, a Join node, because it has no references in the outputs of the Join and cannot impact any predicate inferencing there
| "SELECT regionkey, (select name from nation where false) from region", | ||
| "SELECT regionkey, NULL from region"); | ||
| assertQuery( | ||
| "SELECT regionkey, (select name from nation where nationkey = 5 and mod(nationkey,5) = 1) from region", |
There was a problem hiding this comment.
@jaystarshot The query used here is logically identical to one from the #23660 issue
There was a problem hiding this comment.
But this current only tests execution and not result expectations?
There was a problem hiding this comment.
It asserts that results are identical to those from SELECT regionkey, NULL from region
| effectiveConjuncts.add(rewritten); | ||
| } | ||
| // If equality inference has reduced the predicate to an expression referring to only constants, it does not make sense to pull this predicate up |
There was a problem hiding this comment.
If rewritten has variable references that are in(variables) we do not skip them and pull them up (by adding them to effectiveConjuncts. The problem here is that EqaulityInference is designed to rewrite expressions to just constants (see test), even though, technically this expression does not refer to any variables
However, if the rewritten expression has no variable references, le.g mod(10,5)=1 - this predicate should not be pulled up, to say, a Join node, because it has no references in the outputs of the Join and cannot impact any predicate inferencing there
Description
Fixes #23660
During predicate pushdown through a Join, we were
FALSE, but now is pushed down to both sides of the joinThis results in the optimizer Incorrectly assuming no results will be produced
Fix
Pulling up expressions happens in
EffectivePredicateExtractor#pullExpressionThroughVariables. This was modified to stop pulling up expressions that only refer to constants since this is logically incorrectTest Plan
Contributor checklist
Release Notes