Avoid repeated evaluation of nullable filter conditions - #30993
Conversation
Use IS DISTINCT FROM TRUE to test whether a condition is false or NULL. The previous IS NULL OR NOT rewrite evaluated nondeterministic conditions twice and could discard rows that should always pass the filter.
Keep the existing OR form for deterministic conditions so expression simplification can expose comparison ranges to domain extraction. Use the single-evaluation form only when a condition is nondeterministic.
|
|
||
| private Expression isFalseOrNullPredicate(Session session, Expression expression) | ||
| { | ||
| return Logical.or(new IsNull(expression), not(metadata, getCharVarcharCoercion(session), expression)); |
There was a problem hiding this comment.
or perhaps we could simply use LET here, if the expression is (a) non-trivial (in io.trino.sql.ir.IrExpressions#bindIfNecessary sense), or (b) at least when it's non-deterministic.
I think we should go with (a) but important to check what's the impact on connector predicate pushdown.
There was a problem hiding this comment.
We should change it to do the comparison using IDENTICAL unconditionally and evaluate what impact that has elsewhere. That’s a more straightforward way to check whether a value is false or null.
It aligns with some experiments I was doing a while ago to simplify how the engine reasons about three-values boolean logic across the board.
There was a problem hiding this comment.
I didn't think about that, but it looks indeed as the best option.
Also, the SQL has a direct way to express "e IS TRUE" and "e IS FALSE" predicates, which makes it easier to push such predicates down into connectors.
|
the first commit is titled "Avoid repeated evaluation of nullable filter conditions" (emphasis mine), but the PR seems about non-deterministic conditions. Nullability seems unimportant? Please clarify |
Description
Evaluate nullable nondeterministic conditions once when simplifying IF, searched CASE, and boolean NULLIF filters. The previous
condition IS NULL OR NOT conditionrewrite evaluated a nondeterministic condition twice, allowing an always-true predicate to discard rows.The inner IF can only return false or NULL, so every row must pass the filter. Before the fix, a reproduction returned 763 rows; afterward it returns all 1,000.
Use
condition IS DISTINCT FROM TRUEfor nondeterministic conditions to preserve the false-or-NULL truth table with one evaluation. Deterministic conditions keep the existing OR form so comparison ranges remain available for filter pushdown.Additional context and related issues
Regression tests cover IF, searched CASE, and NULLIF, plus a projection control and ordinary true/false/NULL inputs. Domain tests cover boolean values and pass a rewritten deterministic comparison through expression simplification, verifying the expected nullable range without a residual filter.
Before the production change, the selected rule and query tests had two failures and two passing controls. Afterward, all 14 tests in
TestSimplifyFilterPredicateandTestExpressionspassed. Module validation, including Checkstyle and Airstyle, passed../mvnw -pl core/trino-main -Dtest=TestSimplifyFilterPredicate,TestExpressions -Dair.check.skip-all=true test ./mvnw -pl core/trino-main validateRelease notes
(x) Release notes are required, with the following suggested text: