Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for ANSI SQL syntax in trim function #11236

Merged
merged 1 commit into from
Apr 8, 2022
Merged

Add support for ANSI SQL syntax in trim function #11236

merged 1 commit into from
Apr 8, 2022

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Mar 1, 2022 •
edited
Loading

Description

Add support for ANSI SQL syntax in trim function

<trim function> ::=
  TRIM <left paren>  <trim operands>  <right paren> 

<trim operands> ::=
  [ [ <trim specification>  ] [ <trim character>  ] FROM ] <trim source> 

<trim source> ::=
  <character value expression> 

<trim specification> ::=
    LEADING
  | TRAILING
  | BOTH

<trim character> ::=
  <character value expression>

Documentation

(x) Documentation issue #11347 is filed, and can be handled later.

Release notes

(x) Release notes entries required with the following suggested text:

# General
* Add support for ANSI compliant `trim` function. ({issue}`11236 `)

@cla-bot cla-bot bot added the cla-signed label Mar 1, 2022
@github-actions github-actions bot added the docs label Mar 1, 2022
@ebyhr ebyhr requested a review from kasiafi March 7, 2022 03:51
@ebyhr ebyhr mentioned this pull request Mar 7, 2022
Closed
@ebyhr ebyhr requested a review from martint March 7, 2022 23:43
kasiafi
kasiafi reviewed Mar 8, 2022
View reviewed changes
core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/Trim.java Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java Show resolved Hide resolved
core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java Outdated Show resolved Hide resolved
core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java Outdated Show resolved Hide resolved
@ebyhr ebyhr requested a review from kasiafi March 11, 2022 01:30
kasiafi
kasiafi reviewed Mar 11, 2022
View reviewed changes
Copy link
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.

Thanks for working on this!
I have some more comments.
However, the issue that worries me the most right now is the thing I mentioned in the previous round: #11236 (comment)
Apparently, the semantics of trim functions is broken for char types. I'd rather we didn't add support for new syntax before we fix the underlying logic.

core/trino-parser/src/main/java/io/trino/sql/tree/Trim.java Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/ExpressionTreeRewriter.java Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/Trim.java Show resolved Hide resolved
core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/AggregationAnalyzer.java Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java Outdated Show resolved Hide resolved
...o-main/src/main/java/io/trino/sql/planner/iterative/rule/CanonicalizeExpressionRewriter.java Outdated Show resolved Hide resolved
@ebyhr ebyhr force-pushed the ebi/trim branch 2 times, most recently from ff4fcb4 to b82f08d Compare March 24, 2022 02:20
@ebyhr ebyhr requested a review from kasiafi March 24, 2022 06:07
@kasiafi
Copy link
Member

kasiafi commented Mar 24, 2022 •
edited
Loading

This constructor is used from ExpressionTreeRewriter.

And who uses the rewriter? Trim should not be created in the Planner.

kasiafi
kasiafi reviewed Mar 24, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/sql/analyzer/Analysis.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/Trim.java Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/Trim.java Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/Trim.java Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/Trim.java Outdated Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/TranslationMap.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/relational/SqlToRowExpressionTranslator.java Outdated Show resolved Hide resolved
@kasiafi
Copy link
Member

kasiafi commented Mar 28, 2022

Addressed the comments except for calling Trim constructor without location in TranslationMap. Could you share the condition when doing that?

I'm sorry for not making it clear. I didn't mean that you have to use the constructor directly. What I meant is that if you do defaultRewrite on the Trim node, it will call that constructor underneath.

@ebyhr ebyhr requested a review from kasiafi March 29, 2022 08:51
kasiafi
kasiafi reviewed Apr 7, 2022
View reviewed changes
Copy link
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.

LGTM overall

core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/TranslationMap.java Outdated Show resolved Hide resolved
kasiafi
kasiafi approved these changes Apr 7, 2022
View reviewed changes
Copy link
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.

Approved. Please squash the commits.

@ebyhr
Copy link
Member Author

ebyhr commented Apr 8, 2022

CI #11368

@ebyhr ebyhr merged commit a319b0b into trinodb:master Apr 8, 2022
@ebyhr ebyhr deleted the ebi/trim branch April 8, 2022 12:26
@ebyhr ebyhr mentioned this pull request Apr 8, 2022
Closed
@github-actions github-actions bot added this to the 377 milestone Apr 8, 2022
@mosabua mosabua mentioned this pull request Apr 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

Assignees
No one assigned
Labels
cla-signed docs
Milestone
377
Development

Successfully merging this pull request may close these issues.
3 participants
@ebyhr @kasiafi @martint