Skip to content

Validate literals in expression analyzer#10720

Merged
findepi merged 1 commit intotrinodb:masterfrom
findepi:findepi/validate-literals-in-expression-analyzer-d1fe12
Jan 27, 2022
Merged

Validate literals in expression analyzer#10720
findepi merged 1 commit intotrinodb:masterfrom
findepi:findepi/validate-literals-in-expression-analyzer-d1fe12

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Jan 21, 2022 •
edited
Loading

As a result, optimizer can safely assume a Literal to represent a
value of given type. Also, queries with invalid literal values should be
guaranteed to fail, regardless of expression pruning.

Fixes #10719
Fixes #10755

@cla-bot cla-bot bot added the cla-signed label Jan 21, 2022
kasiafi
kasiafi reviewed Jan 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@kasiafi kasiafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please cover also:

  • GenericLiteral with type JSON,
  • IntervalLiteral

In fact, you could use LiteralInterpreter for the validation.

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jan 21, 2022

In fact, you could use LiteralInterpreter for the validation.

I thought about that, i chose not to, because i already have resolved coercion at hand, so LiteralInterpreter didn't buy me much.

@findepi findepi mentioned this pull request Jan 21, 2022
Closed
@findepi findepi force-pushed the findepi/validate-literals-in-expression-analyzer-d1fe12 branch from 4ee442d to d3670d8 Compare January 21, 2022 11:13
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jan 21, 2022

In fact, you could use LiteralInterpreter for the validation.

switched over to LiteralInterpreter, as otherwise JSON verification would be cumbersome

Please cover also:

  • GenericLiteral with type JSON,
  • IntervalLiteral

added

findepi
findepi commented Jan 21, 2022
View reviewed changes
core/trino-main/src/test/java/io/trino/sql/analyzer/TestAnalyzer.java Outdated
Copy link
Copy Markdown
Member Author

@findepi findepi Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#10724

@findepi findepi mentioned this pull request Jan 21, 2022
Merged
@findepi findepi force-pushed the findepi/validate-literals-in-expression-analyzer-d1fe12 branch from d3670d8 to bc3af3f Compare January 21, 2022 15:06
findepi
findepi commented Jan 21, 2022
View reviewed changes
core/trino-main/src/test/java/io/trino/type/TestDecimalOperators.java Outdated
Comment on lines 195 to 196
Copy link
Copy Markdown
Member Author

@findepi findepi Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#10732

@findepi findepi requested a review from kasiafi January 21, 2022 15:07
@findepi findepi force-pushed the findepi/validate-literals-in-expression-analyzer-d1fe12 branch from bc3af3f to bc0899b Compare January 21, 2022 20:31
findepi
findepi commented Jan 21, 2022
View reviewed changes
testing/trino-tests/src/test/java/io/trino/tests/AbstractTestEngineOnlyQueries.java Outdated
Copy link
Copy Markdown
Member Author

@findepi findepi Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a behavioral change, but IMO justified.

#10755

testing/trino-tests/src/test/java/io/trino/tests/AbstractTestEngineOnlyQueries.java Outdated
Copy link
Copy Markdown
Member Author

@findepi findepi Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a behavioral change -- see above.

@kasiafi kasiafi mentioned this pull request Jan 25, 2022
Merged
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jan 27, 2022

rebased to resolve conflict with #10737

@findepi findepi force-pushed the findepi/validate-literals-in-expression-analyzer-d1fe12 branch 2 times, most recently from 0a7d720 to 26dbcac Compare January 27, 2022 13:56
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jan 27, 2022

To be rebased after #10783 is merged.

@findepi findepi force-pushed the findepi/validate-literals-in-expression-analyzer-d1fe12 branch from 26dbcac to 4d65d6c Compare January 27, 2022 14:59
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jan 27, 2022

rebased after #10783 merged (no changes yet)

@findepi
Validate literals in expression analyzer
d4dc855 
As a result, optimizer can safely assume a `Literal` to represent a
value of given type. Also, queries with invalid literal values should be
guaranteed to fail, regardless of expression pruning.
@findepi findepi force-pushed the findepi/validate-literals-in-expression-analyzer-d1fe12 branch from 4d65d6c to d4dc855 Compare January 27, 2022 15:07
@findepi findepi merged commit c8540c7 into trinodb:master Jan 27, 2022
@findepi findepi deleted the findepi/validate-literals-in-expression-analyzer-d1fe12 branch January 27, 2022 20:14
@github-actions github-actions bot added this to the 370 milestone Jan 27, 2022
This was referenced Jan 27, 2022
Merged
Closed
@findepi findepi mentioned this pull request Feb 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kasiafi kasiafi kasiafi approved these changes

@martint martint Awaiting requested review from martint

@losipiuk losipiuk Awaiting requested review from losipiuk

@kokosing kokosing Awaiting requested review from kokosing

Assignees

No one assigned

Labels

cla-signed

Milestone

370

Development

Successfully merging this pull request may close these issues.

Inconsistent handling of invalid literals within in try(...) Literals should be validated before optimizer

2 participants

@findepi @kasiafi