Apply coercions when creating FilterNode #15744

findepi · 2023-01-17T16:24:48Z

When predicate is not of boolean type (e.g. null literal being of
unknown type), the Analysis holds a coercion that should be applied
to it. Before the change, the coercion was not getting applied though.

findepi · 2023-01-17T22:28:09Z

cc @assaf2 @martint

assaf2 · 2023-01-18T09:08:52Z

core/trino-main/src/main/java/io/trino/sql/planner/plan/FilterNode.java

Should we do something more generic here? Like checking if predicate is of type Literal but not a BooleanLiteral?

We can make it more complicated, yes.
However we cannot analyze expression type fully, as we don't have symbol types here, so we fail short of being really generic. I draw the line here, we can draw the line somewhere else.

I would not add the check for NullLiteral. We don't do this in other PlanNodes which require boolean expressions.

I think we can all agree that FilterNode.predicate should be of boolean type.
Yet, the code base required updates in quite a few places to bring reality in line with the design.
The only known to me way to effectively find those places was to add the check here.
Now, we can hope that codebase is now correct and we can remove the check, but how will we ensure the problems do not come back with some new code?

I can see that the PlanNode constructor seems to be the best place for the check. If we do the check immediately after the initial planning, we might miss wrong filters created in the Optimizer. If we wait until execution, we might miss / break optimizations.

This check is not satisfying though, as it only detects NullLiteral. If you want to keep it, then maybe add a TODO explaining what's missing. That TODO will be easy to address in the future, when we have the new IR with types included.

Also, consider adding a similar check in other PlanNodes, at least those covered by this PR.

This check is not satisfying though, as it only detects NullLiteral.

Correct, the check is a required check, but doesn't guarantee well-formedness.

consider adding a similar check in other PlanNodes

I would prefer to defer that, if i were to choose

If you want to keep it, then maybe add a TODO explaining what's missing.

Good idea, will add a comment!

assaf2 · 2023-01-18T09:13:43Z

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

Regarding the connection of this PR and #15558, can you also remove the handling of this case from RemoveTrivialFilters and then whoever gets the conflict will solve it, or do you have other plans?

can you also remove the handling of this case from RemoveTrivialFilters

Before my commit, there are cases where RemoveTrivialFilters can remove null filter -- it's when it's written as NULL in SQL and coercions are not applied. So changing RemoveTrivialFilters within this PR would be removing existing optimization. However, applying the coercions also removes the existing optimization, as the optimization no longer kicks in (becomes dead code)

Huh, I think your change needs to go first.

If we don't add the check in FilterNode constructor, like I suggested, we could retain this test case, and let it fail because of checkArgument in RemoveTrivialFilters. However, I think that the Optimizer rule should not validate the node, but instead just ignore NullLiteral.

I think that for now we can skip the validation altogether. That would be consistent with how we handle other sites where boolean type is required. If we want to validate the expression types, we should introduce a new mechanism that would have access to full type information (including symbols), and apply it to each site, that is: filter, joins, aggregation filter, merge etc.

However, I think that the Optimizer rule should not validate the node, but instead just ignore NullLiteral.

Yes, a rule doesn't need to validate a node, but it also is free to fail when a node doesn't adhere to a design. For example, a ProjectNode cannot have multiple sources, so a rule can simply fail if one does.

I think that for now we can skip the validation altogether. That would be consistent with how we handle other sites where boolean type is required

answered at the other comment: 020b366#r1085170555

findepi · 2023-01-23T10:19:05Z

CI #15809

This is sometimes useful, so let's always do it.

findepi · 2023-01-24T15:45:02Z

Thank you for your review! AC, PTAL

This is for code consistency reasons. Most of the places where `ExpressionMatcher` is constructed already goes through `PlanMatchPattern.expression` factory method.

`UnwrapCastInComparison` works on any nested comparison expressions.

findepi · 2023-01-25T16:30:52Z

Thank you for your review! AC, PTAL

findepi · 2023-01-26T16:20:02Z

CI #15793

`StatementAnalyzer.Visitor#analyzeRowFilter` captures necessary coercions for filters that are not of boolean type (e.g. `null` literal being of `unknown` type). Before the change, the coercions where not getting applied though.

When predicate is not of boolean type (e.g. `null` literal being of `unknown` type), the `Analysis` holds a coercion that should be applied to it. Before the change, the coercion was not getting applied though. Co-authored-by: Assaf Bern <[email protected]>

sopel39 · 2023-01-26T20:58:07Z

Is there a potential perf impact? Does it need benchmarks?

findepi · 2023-01-27T08:46:24Z

Is there a potential perf impact? Does it need benchmarks?

@sopel39 good question, thanks for thinking about this!

There is some possibility that certain filters now become pruned out where they didn't before.
However, this is for edge cases only. The TPC-H/DS plans didn't change, so benchmarks results wouldn't either.

findepi · 2023-01-27T08:49:11Z

CI #15793, #15770

sopel39 · 2023-01-27T11:33:13Z

However, this is for edge cases only. The TPC-H/DS plans didn't change, so benchmarks results wouldn't either.

I was thinking that maybe execution now could take longer?

findepi · 2023-01-27T13:35:07Z

you mean that the generated code is different? this i don't know, unfortunately.
@dain would be the best person to answer this.

sopel39 · 2023-01-27T14:25:04Z

@findepi would you like to run benchmarks?

cla-bot bot added the cla-signed label Jan 17, 2023

findepi mentioned this pull request Jan 17, 2023

Remove constant null filters #15558

Closed

findepi force-pushed the findepi/test-predicates-never-of-unknown-type-7d70de branch 2 times, most recently from d53d8a3 to 605703b Compare January 18, 2023 09:02

findepi changed the title ~~Test predicates never of unknown type~~ Apply coercions when creating FilterNode Jan 18, 2023

findepi marked this pull request as ready for review January 18, 2023 09:02

findepi requested review from assaf2, kokosing, martint and sopel39 January 18, 2023 09:03

findepi added the no-release-notes This pull request does not require release notes entry label Jan 18, 2023

assaf2 reviewed Jan 18, 2023

View reviewed changes

sopel39 requested review from kasiafi and removed request for sopel39 January 18, 2023 10:02

findepi mentioned this pull request Jan 18, 2023

Add support for CHECK constraint in INSERT statement in engine and SPI #14964

Merged

findepi force-pushed the findepi/test-predicates-never-of-unknown-type-7d70de branch 2 times, most recently from 605703b to 1afd5e3 Compare January 18, 2023 12:46

findepi force-pushed the findepi/test-predicates-never-of-unknown-type-7d70de branch from 1afd5e3 to da8ef8d Compare January 23, 2023 10:20

This comment was marked as outdated.

Sign in to view

findepi requested a review from sopel39 January 23, 2023 10:26

findepi force-pushed the findepi/test-predicates-never-of-unknown-type-7d70de branch from da8ef8d to 5389bc0 Compare January 24, 2023 08:53

This comment was marked as outdated.

Sign in to view

findepi added 2 commits January 24, 2023 10:27

Attach query when plan assertion fails

0d7fd68

This is sometimes useful, so let's always do it.

Prevent accidental modifications of analysis masks and filters

e920b53

findepi force-pushed the findepi/test-predicates-never-of-unknown-type-7d70de branch 2 times, most recently from 4914b03 to 020b366 Compare January 24, 2023 09:37

findepi requested a review from assaf2 January 24, 2023 09:39

findepi force-pushed the findepi/test-predicates-never-of-unknown-type-7d70de branch from 039b2fa to 09c97f2 Compare January 24, 2023 15:44

findepi requested a review from kasiafi January 24, 2023 15:45

findepi force-pushed the findepi/test-predicates-never-of-unknown-type-7d70de branch from 09c97f2 to d7d7e95 Compare January 24, 2023 16:01

findepi added 3 commits January 25, 2023 17:21

Annotate PlanMatchPattern method arguments as SQL

87f2a55

Encapsulate ExpressionMatcher construction

d1808f1

This is for code consistency reasons. Most of the places where `ExpressionMatcher` is constructed already goes through `PlanMatchPattern.expression` factory method.

Test unwrapping within nested expressions

81dd9d7

`UnwrapCastInComparison` works on any nested comparison expressions.

findepi force-pushed the findepi/test-predicates-never-of-unknown-type-7d70de branch from d7d7e95 to 76c3905 Compare January 25, 2023 16:30

findepi force-pushed the findepi/test-predicates-never-of-unknown-type-7d70de branch from 76c3905 to f59e623 Compare January 26, 2023 10:00

assaf2 approved these changes Jan 26, 2023

View reviewed changes

findepi and others added 3 commits January 26, 2023 20:51

Add more null filter related tests in TestLogicalPlanner

da0f434

Add coercions for row filters

246d52a

`StatementAnalyzer.Visitor#analyzeRowFilter` captures necessary coercions for filters that are not of boolean type (e.g. `null` literal being of `unknown` type). Before the change, the coercions where not getting applied though.

findepi force-pushed the findepi/test-predicates-never-of-unknown-type-7d70de branch from f59e623 to 402fc7b Compare January 26, 2023 19:52

losipiuk approved these changes Jan 26, 2023

View reviewed changes

findepi merged commit dc40577 into trinodb:master Jan 27, 2023

findepi deleted the findepi/test-predicates-never-of-unknown-type-7d70de branch January 27, 2023 08:52

findepi removed the no-release-notes This pull request does not require release notes entry label Jan 27, 2023

findepi mentioned this pull request Jan 27, 2023

Release notes for 407 #15854

Closed

github-actions bot added this to the 407 milestone Jan 27, 2023

colebow mentioned this pull request Jan 31, 2023

Add Trino 407 release notes #15919

Merged

takezoe mentioned this pull request Aug 5, 2023

Coerce boolean type for filter expression #18551

Merged

Apply coercions when creating FilterNode #15744

Apply coercions when creating FilterNode #15744

Uh oh!

Conversation

findepi commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented Jan 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!

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 Jan 23, 2023

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

findepi commented Jan 24, 2023

Uh oh!

findepi commented Jan 25, 2023

Uh oh!

findepi commented Jan 26, 2023

Uh oh!

sopel39 commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented Jan 27, 2023

Uh oh!

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

Uh oh!

Uh oh!

sopel39 commented Jan 27, 2023

Uh oh!

findepi commented Jan 27, 2023

Uh oh!

sopel39 commented Jan 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

findepi commented Jan 17, 2023 •

edited

Loading

sopel39 commented Jan 26, 2023 •

edited

Loading

findepi commented Jan 27, 2023 •

edited

Loading