Skip to content

[CALCITE-6643] Char_Length Function is not recognized in PrestoSql. Add LENGTH function in PrestoSqlDialect#4017

Merged
mihaibudiu merged 1 commit intoapache:mainfrom
vikramahuja1001:PrestoSqlLengthFix
Oct 25, 2024
Merged

[CALCITE-6643] Char_Length Function is not recognized in PrestoSql. Add LENGTH function in PrestoSqlDialect#4017
mihaibudiu merged 1 commit intoapache:mainfrom
vikramahuja1001:PrestoSqlLengthFix

Conversation

@vikramahuja1001
Copy link
Copy Markdown
Contributor

@vikramahuja1001 vikramahuja1001 commented Oct 25, 2024

PrestoSqlDialect by default will change any length function in a query to char_length function but char_length function is not available/supported in PrestoSql.
Add length function support in PrestoSqlDialect

@vikramahuja1001 vikramahuja1001 changed the title [Wip] Add Length Function in PrestoSqlDialect [CALCITE-6643] Add Length Function in PrestoSqlDialect Oct 25, 2024
@mihaibudiu
Copy link
Copy Markdown
Contributor

the commit message should match the JIRA issue and PR title
usually you start with the JIRA issue

@vikramahuja1001 vikramahuja1001 changed the title [CALCITE-6643] Add Length Function in PrestoSqlDialect [CALCITE-6643] Char_Length Function is not recognized in PrestoSql. Add length function in PrestoSqlDialect Oct 25, 2024
@vikramahuja1001 vikramahuja1001 changed the title [CALCITE-6643] Char_Length Function is not recognized in PrestoSql. Add length function in PrestoSqlDialect [CALCITE-6643] Char_Length Function is not recognized in PrestoSql. Add LENGTH function in PrestoSqlDialect Oct 25, 2024
@vikramahuja1001
Copy link
Copy Markdown
Contributor Author

vikramahuja1001 commented Oct 25, 2024

@mihaibudiu i have updated the same. Thanks for letting me know.

Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

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

Haven't checked the presto spec, but assuming this is right, the PR looks fine.

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Oct 25, 2024
Copy link
Copy Markdown
Member

@caicancai caicancai left a comment

Choose a reason for hiding this comment

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

overall LGTM

@sonarqubecloud
Copy link
Copy Markdown

@mihaibudiu mihaibudiu merged commit 25247c1 into apache:main Oct 25, 2024
@vikramahuja1001
Copy link
Copy Markdown
Contributor Author

Thanks for the review @mihaibudiu , @caicancai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants