Add a new rule for removing "false" filters by assaf2 · Pull Request #16206 · trinodb/trino

assaf2 · 2023-02-21T17:43:25Z

Description

The rule uses DomainTranslator to infer that the expression is "false".

Additional context and related issues

Release notes

( X ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

findepi · 2023-02-27T11:28:56Z

...trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushPredicateIntoTableScan.java

The reason why you're adding a check here is that such Filter wasn't removed by some other rule.
What if a null-filter eligible for removal is not directly above a Table Scan?
My understanding is that it won't get removed, but it should.

I think we need a check here, in PushPredicateIntoTableScan, but we definitely need a check elsewhere as well.

I don't think there's a way to come up with a rule that can cover 100% of the cases because:

AFAIU we don't want to evaluate the entire expression within the rule (source: Remove constant null filters #15558 (comment)).

Comparing (except IS DISTINCT FROM) null to anything should result in null, not false (source: https://trino.io/docs/current/functions/comparison.html#is-distinct-from-and-is-not-distinct-from). I can modify io.trino.sql.planner.ExpressionInterpreter.Visitor#processComparisonExpression to return false instead of null when left \ right is null, but AFAIU this is not logically correct (BTW I've tried this and got lots of tests failures).

I'm not sure I agree with the comment that existed even before my change:

trino/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushPredicateIntoTableScan.java

Lines 237 to 239 in 208b380

// TODO: DomainTranslator.fromPredicate can infer that the expression is "false" in some cases (TupleDomain.none()).

// This should move to another rule that simplifies the filter using that logic and then rely on RemoveTrivialFilters

// to turn the subtree into a Values node

Doing the same logic twice just to spare this if? And what if the logic of PushPredicateIntoTableScan will be changed in the future? If we can't come up with a rule that can cover 100% of the filters that are eventually evaluated to false/ null, then IMHO, we should rely on this if. WDYT?

AFAIU we don't want to evaluate the entire expression within the rule (source: #15558 (comment)).

We're not evaluating here either, right?

What prevents us from having a Rule that would e.g. apply to every FilterNode and did the logic which we're doing here?

We're not evaluating here either, right?

Right, we're executing some logic that's specific for the pushdown and discovering false filters on the way.

What prevents us from having a Rule that would e.g. apply to every FilterNode and did the logic which we're doing here?

It feels arbitrary to me to execute some logic that we happen to have that's doing part of the job, but that must be a minority opinion since both you and the comment writer think otherwise.

I think we need a check here, in PushPredicateIntoTableScan, but we definitely need a check elsewhere as well.

So I'll leave this if as is and add another rule

As in #16206 (comment), i think we need both

a separate rule, which would cover for Filter-not-necessarily-right-above-a-TableScan that is redundant and can be removed

the change here as proposed is also fine to have. Rules should avoid depending on each other as much as possible. However, we should strive so that reverting the change here in PushPredicateIntoTableScan.java would not impact what query plans we produce.

assaf2 · 2023-03-23T15:32:59Z

Rebased in order to resolve conflicts

assaf2 · 2023-03-26T17:36:53Z

@findepi @martint PTAL

findepi · 2023-03-28T07:52:04Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

We now have two triggering conditions for the exception.
If the exception is raised, it would be good to know which one triggered. Add constraint to the message

with that commit being first, do all the test pass?

The check here was for added for documentation purposes 83e144a

Fortunately, the applyFilter is never invoked with NONE domain, and
should not be, since the engine knows what to do in such case without
asking the connector.

The || FALSE.equals(constraint.getExpression() doesn't seem to be just for documentation purposes though, since there is no equivalent logic in PushPredicateIntoTableScan.

If current PushPredicateIntoTableScan code guarantees that FALSE.equals(constraint.getExpression() will never be true, this can be as is, but requires a code comment. If we go that way, I would split the if into two independent ones (the old condition and the new condition do not deserve same type of explanation)

with that commit being first, do all the test pass?

The issue was with NULL pushdown, not with FALSE, all tests should pass regardless its position among the other commits. Sorry if it caused confusion, do you want me to open a separate PR for this change?
I saw that we validated summary is non-NONE and thought this validation is not whole without checking that the other part of the constraint (the expression) is also non-FALSE.

If current PushPredicateIntoTableScan code guarantees that FALSE.equals(constraint.getExpression() will never be true

There's no explicit check for that, but DomainTranslator's implementation translates FALSE expressions into TupleDomain.none() (via visitBooleanLiteral) so the remaining expression shouldn't be FALSE and therefore the translated connector expression shouldn't be FALSE as well. Do you think I better add an explicit check within PushPredicateIntoTableScan?

The check here was for added for documentation purposes 83e144a

Fortunately, the applyFilter is never invoked with NONE domain, and

I was thinking that if we don't want it to be invoked with NONE domain (which is reasonable - why would we want to pushdown NONE filter into the connector?), then we probably don't want to pushdown FALSE expressions as well. I don't see a separation here between documentation and validation. If we say for documentation purposes that summary shouldn't be NONE then why not say that expression shouldn't be FALSE? I guess we don't want inconsistent behavior between the two. As mentioned earlier, I can add an explicit check to PushPredicateIntoTableScan if you think it's necessary.

We now have two triggering conditions for the exception.
If the exception is raised, it would be good to know which one triggered.

Agree, I separated it into 2 different if clauses with different exception messages.

core/trino-main/src/main/java/io/trino/sql/planner/DomainTranslator.java

...o-main/src/test/java/io/trino/sql/planner/iterative/rule/TestPushPredicateIntoTableScan.java

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java

...no-main/src/main/java/io/trino/sql/planner/iterative/rule/RemoveEmptyMergeWriterRuleSet.java

findepi · 2023-03-28T08:06:46Z

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

RemoveTrivialFilters is used twice, the new rule is used once. what's the selection criteria to when to use the new rule and when not to?

I wanted to avoid PushPredicateIntoTableScan from getting into this if clause

if (newDomain.isNone()) { // This is just for extra safety, the rule RemoveFalseFiltersAfterDomainTranslator is responsible for eliminating such filters

I saw that we run simplifyOptimizer at line 556 and then PushPredicateIntoTableScan for the first time at line 600 so I thought it was not needed at line 438 as well. But thinking about it again, if we have a chance of getting a trivial filter at line 438, maybe it's worth removing FALSE filters at this point as well. Added.

...trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushPredicateIntoTableScan.java

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

...ain/src/test/java/io/trino/sql/planner/iterative/rule/TestRemoveEmptyMergeWriterRuleSet.java

core/trino-main/src/main/java/io/trino/sql/planner/DomainTranslator.java

findepi · 2023-04-12T09:51:41Z

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

did you mean to invoke RemoveTrivialFilters as well?

As mentioned at #16206 (comment) I've added this line because I saw PushPredicateIntoTableScan at PlanOptimizers:600 may produce None predicates. IMO this realization is insufficient to determine that RemoveTrivialFilters invocation is also required. Do you think otherwise?

I don't understand. Perhaps you need to explain this to me once more.
I am just curious what's the usefulness of RemoveFalseFiltersAfterDomainTranslator if not coming together with RemoveTrivialFilters. It won't remove the FilterNode.

You are right, now that RemoveFalseFiltersAfterDomainTranslator is an ExpressionRewriteRuleSet and not a standalone rule, RemoveTrivialFilters is also required. Added.

findepi · 2023-04-12T09:51:46Z

...c/main/java/io/trino/sql/planner/iterative/rule/RemoveFalseFiltersAfterDomainTranslator.java

I think the new rule should extend ExpressionRewriteRuleSet and thus apply to any boolean expression that can be proven to be false. This would apply to filternode, but also to projections (IF, CASE or just projecting the result of a boolean expression).

after switching to ExpressionRewriteRuleSet "RemoveFalseFiltersAfterDomainTranslator" is no longer a good rule name.

"SimplifyFalseConditions"

findepi · 2023-04-12T09:52:54Z

...c/main/java/io/trino/sql/planner/iterative/rule/RemoveFalseFiltersAfterDomainTranslator.java

Another issue - we are now translating any CAST(null AS <type>) to FALSE although when not within a filter, the expression sometimes should stay null. For example, run io.trino.plugin.postgresql.TestPostgreSqlTableStatistics#testNullsFraction

correct.

is postgresql test the only one catching that?
please add something to AbstractTestEngineOnlyQueries as well

findepi · 2023-04-12T09:55:54Z

This PR is growing. It's awesome to see more good changes coming.
At the same time, it's becoming harder to re-review.

Can you try moving Handle cast NULL in DomainTranslator to an own PR (may be with the first 3 commits)?

Can you try moving Add error handling to a coercion to a separate PR? (perhaps calling it "Add error handling for saturated floor cast")

assaf2 · 2023-04-13T09:47:56Z

This PR is growing. It's awesome to see more good changes coming. At the same time, it's becoming harder to re-review.

Can you try moving Handle cast NULL in DomainTranslator to an own PR (may be with the first 3 commits)?

Can you try moving Add error handling to a coercion to a separate PR? (perhaps calling it "Add error handling for saturated floor cast")

Done. See #17013 and #17014.
Please note that this PR is now blocked by #17014.

findepi · 2023-04-13T13:12:49Z

Done. See #17013 and #17014.

thanks, reviewing these.

assaf2 · 2023-04-16T12:43:06Z

Done. See #17013 and #17014.

thanks, reviewing these.

Thank you, I decided to move few more commits into 2 more PRs- #17047 and #17048

Make Project optional, for the case MergeProjectWithValues is executed beforehand.

The rule uses DomainTranslator to infer that the expression is "false".

assaf2 · 2023-04-17T13:33:03Z

Rebased onto master branch due to conflicts

martint · 2023-04-17T18:03:32Z

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

                                .addAll(columnPruningRules)
                                .add(new InlineProjections(plannerContext, typeAnalyzer))
                                .addAll(new PushFilterThroughCountAggregation(plannerContext).rules()) // must run after PredicatePushDown and after TransformFilteringSemiJoinToInnerJoin
+                                .addAll(new SimplifyFalseConditions(plannerContext).rules())


There's already a rule for this: RemoveTrivialFilters. The new logic should be implemented within that rule.

i think it became a separate rule following #15558 (comment)

I think it was mainly because of @martint's comment from 4 years ago:

trino/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushPredicateIntoTableScan.java

Lines 241 to 243 in 2ddb5d8

// TODO: DomainTranslator.fromPredicate can infer that the expression is "false" in some cases (TupleDomain.none()).

// This should move to another rule that simplifies the filter using that logic and then rely on RemoveTrivialFilters

// to turn the subtree into a Values node

But I don't object to merging these two, they seem to go together anyway + that will turn RemoveTrivialFilters into an ExpressionRewriteRuleSet so maybe it will also benefit the join case (I didn't check).
I'm not sure the term "trivial" will still be accurate though.

It's ok to have a rule that simplifies expressions (we already have one) and one that removes filters where the expression is known to filter everything or nothing (we already have one, too). But we should not add yet another rule that mixes both concerns.

I took another look, RemoveTrivialFilters can't become an ExpressionRewriteRuleSet because ExpressionRewriteRuleSet doesn't replace a node of one type with a node of another type.
The new rule's logic can't be moved into SimplifyExpressions because not all node types are supported (see #16206 (comment)).
I can make RemoveTrivialFilters return 2 rules, but I failed to prove the join case (couldn't find an actual query that has a final plan that contains a JoinNode with a filter that can be simplified to FALSE \ TRUE).
@findepi \ @martint WDYT should be the next step?

the join case (couldn't find an actual query that has a final plan that contains a JoinNode with a filter that can be simplified to FALSE \ TRUE).

I guess that's because PPD pushes such filter under the join and then it gets simplified/eliminated there

But we should not add yet another rule that mixes both concerns.

I read it as implying RemoveTrivialFilters shouldn't call DomainTranslator for help.

We tried to introduce a generic rule ExpressionRewriteRuleSet-based to simplify boolean conditions that can be replaced with constant false. I know what problems this run into (distinguishing context: eg between projected).
Did it result in more test coverage? like cases mentioned in #16206 (comment) ?

I read it as implying RemoveTrivialFilters shouldn't call DomainTranslator for help.

Please see #16206 (comment). Note that adding this logic into RemoveTrivialFilters will address this concern you raised #16206 (comment).

Did it result in more test coverage? like cases mentioned in #16206 (comment) ?

I didn't add new tests for those cases.

I didn't add new tests for those cases.

Can you please do?
These are interesting test cases to have, regardless of the implementation choices we end up doing in this PR

findepi · 2023-04-25T09:49:37Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/SimplifyFalseConditions.java

+                types);
+
+        if (decomposedPredicate.getTupleDomain().isNone()) {
+            return FALSE_LITERAL;


what if the expression was already a false_literal? do we need to return expression in such case (same, not just equal)?

I don't see a reason for that, do you have something in mind?

old

github-actions · 2024-01-17T17:03:28Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-01-17T19:24:30Z

👋 @assaf2 @findepi @martint - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

github-actions · 2024-02-12T17:33:49Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

github-actions · 2024-03-05T17:11:09Z

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

cla-bot bot added the cla-signed label Feb 21, 2023

assaf2 force-pushed the avoid-null-predicate-pushdown branch from 9ad296e to 8bbceb3 Compare February 21, 2023 17:45

assaf2 requested review from findepi and martint February 22, 2023 07:57

assaf2 marked this pull request as ready for review February 22, 2023 07:57

assaf2 self-assigned this Feb 26, 2023

findepi previously requested changes Feb 27, 2023

View reviewed changes

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

assaf2 mentioned this pull request Mar 23, 2023

Table scan not getting eliminated for a predicate non-satisfiable due to a CAST domain #16515

Closed

assaf2 force-pushed the avoid-null-predicate-pushdown branch 2 times, most recently from 598fa26 to 233f49e Compare March 23, 2023 15:32

assaf2 force-pushed the avoid-null-predicate-pushdown branch from 233f49e to 140f9b9 Compare March 23, 2023 19:32

assaf2 marked this pull request as draft March 23, 2023 19:34

assaf2 force-pushed the avoid-null-predicate-pushdown branch 5 times, most recently from b9446e5 to 869b5c5 Compare March 26, 2023 17:36

assaf2 marked this pull request as ready for review March 26, 2023 17:36

findepi reviewed Mar 28, 2023

View reviewed changes

assaf2 force-pushed the avoid-null-predicate-pushdown branch from 869b5c5 to ae1ae9a Compare March 28, 2023 15:35

github-actions bot added the iceberg Iceberg connector label Mar 28, 2023

assaf2 marked this pull request as draft March 29, 2023 06:23

assaf2 force-pushed the avoid-null-predicate-pushdown branch 3 times, most recently from ce1a237 to 5f74783 Compare March 31, 2023 06:47

findepi marked this pull request as ready for review March 31, 2023 09:18

findepi reviewed Mar 31, 2023

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java Outdated Show resolved Hide resolved

assaf2 requested a review from findepi April 2, 2023 13:41

assaf2 force-pushed the avoid-null-predicate-pushdown branch 2 times, most recently from 50920fc to 1286d74 Compare April 2, 2023 14:08

findepi reviewed Apr 12, 2023

View reviewed changes

assaf2 force-pushed the avoid-null-predicate-pushdown branch from 1286d74 to 0c2ac27 Compare April 13, 2023 09:22

assaf2 mentioned this pull request Apr 13, 2023

Add error handling for saturated floor cast #17014

Merged

assaf2 force-pushed the avoid-null-predicate-pushdown branch from 0c2ac27 to 5ae6600 Compare April 13, 2023 09:47

assaf2 changed the title ~~Avoid null predicate pushdown~~ Add a new rule for removing "false" filters Apr 13, 2023

assaf2 requested a review from findepi April 13, 2023 09:52

assaf2 mentioned this pull request Apr 16, 2023

Handle cast null in domain translator #17013

Merged

assaf2 force-pushed the avoid-null-predicate-pushdown branch from a05aa88 to 703d528 Compare April 16, 2023 15:32

assaf2 added 2 commits April 17, 2023 16:31

Remove empty MergeWriter also when no Project

1b34f05

Make Project optional, for the case MergeProjectWithValues is executed beforehand.

Add a new rule for removing "false" filters

041d25c

The rule uses DomainTranslator to infer that the expression is "false".

assaf2 force-pushed the avoid-null-predicate-pushdown branch from 703d528 to 041d25c Compare April 17, 2023 13:32

martint reviewed Apr 17, 2023

View reviewed changes

findepi reviewed Apr 25, 2023

View reviewed changes

github-actions bot added the stale label Jan 17, 2024

github-actions bot removed the stale label Jan 18, 2024

github-actions bot added the stale label Feb 12, 2024

github-actions bot closed this Mar 5, 2024

	// TODO: DomainTranslator.fromPredicate can infer that the expression is "false" in some cases (TupleDomain.none()).
	// This should move to another rule that simplifies the filter using that logic and then rely on RemoveTrivialFilters
	// to turn the subtree into a Values node

Conversation

assaf2 commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

assaf2 Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

assaf2 commented Mar 23, 2023

Uh oh!

assaf2 commented Mar 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Apr 12, 2023

Uh oh!

assaf2 commented Apr 13, 2023

Uh oh!

findepi commented Apr 13, 2023

Uh oh!

assaf2 commented Apr 16, 2023

Uh oh!

assaf2 commented Apr 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

assaf2 Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

assaf2 commented Feb 21, 2023 •

edited

Loading

assaf2 Feb 27, 2023 •

edited

Loading

assaf2 Apr 25, 2023 •

edited

Loading