Skip to content

[sqlparser-0.21] Update trimExpr members during planning - #3181

Merged
alamb merged 2 commits into
apache:sqlparser-0.21from
ayushdg:update-trimexpr-sqlparser
Aug 19, 2022
Merged

[sqlparser-0.21] Update trimExpr members during planning#3181
alamb merged 2 commits into
apache:sqlparser-0.21from
ayushdg:update-trimexpr-sqlparser

Conversation

@ayushdg

@ayushdgayushdg commented Aug 16, 2022

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Extension of apache/datafusion-sqlparser-rs#568 into datafusion

Rationale for this change

Updates were made to sqlparser parsing logic apache/datafusion-sqlparser-rs#573 to handle optional trim flag and trim what Expr's better matching postgresql syntax.

What changes are included in this PR?

This pr updates the planner logic to handle the changes made to the struct members of Expr::Trim from sqlparser.

Are there any user-facing changes?


Note: Errors are related to other changes made in sqlparser around the handling of like/ilike being addressed in #3101. Might need to consolidate both the changes in a single pr for tests to pass.

@github-actionsgithub-actionsBot added core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner labels Aug 16, 2022
@andygrove

Copy link
Copy Markdown
Member

@ayushdg Could you target this to the sqlparser-0.21 branch? We'll need to merge PRs there in the order specified in #3192

Also, see discussion on this feature branch approach in #3191

@andygroveandygrove mentioned this pull request Aug 18, 2022
5 tasks
@ayushdg
ayushdg changed the base branch from master to sqlparser-0.21August 18, 2022 17:49
@ayushdg
ayushdg marked this pull request as ready for review August 18, 2022 17:50
@ayushdgayushdg changed the title [WIP] Update trimExpr members during planningUpdate trimExpr members during planningAug 18, 2022
@andygroveandygrove changed the title Update trimExpr members during planning[sqlparser-0.21] Update trimExpr members during planningAug 18, 2022
@andygrove

Copy link
Copy Markdown
Member

@ayushdg Could you please rebase against sqlparser-0.21 branch and change the sqlparser rev to 42c5d43b45d3e7a573ac24dd5c927c43bbd3768c. This is the next one to go in.

@ayushdg
ayushdgforce-pushed the update-trimexpr-sqlparser branch from bd827f6 to 1e46ff8CompareAugust 18, 2022 23:17
@ayushdg

Copy link
Copy Markdown
ContributorAuthor

@ayushdg Could you please rebase against sqlparser-0.21 branch and change the sqlparser rev to 42c5d43b45d3e7a573ac24dd5c927c43bbd3768c. This is the next one to go in.

Done @andygrove! Lemme know if there's anything else needed for this pr.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #3181 (1e46ff8) into sqlparser-0.21 (d4fefc7) will decrease coverage by 0.00%.
The diff coverage is 70.00%.

@@ Coverage Diff @@## sqlparser-0.21 #3181 +/- ##
==================================================
- Coverage 85.84% 85.84% -0.01% 
==================================================
Files 291 291 Lines 52898 52898 ==================================================
- Hits 45411 45410 -1 - Misses 7487 7488 +1 
Impacted FilesCoverage Δ
datafusion/sql/src/planner.rs80.61% <70.00%> (-0.14%)⬇️
datafusion/expr/src/logical_plan/plan.rs77.95% <0.00%> (+0.34%)⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@andygroveandygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks @ayushdg

@andygrove

Copy link
Copy Markdown
Member

It would be good to add some tests as well either in this PR or as a follow on PR.

@alamb
alamb merged commit 292ff5b into apache:sqlparser-0.21Aug 19, 2022
alamb pushed a commit that referenced this pull request Aug 19, 2022
* Changes to planning for SHOW TABLES due to changes in sqlparser (#3193)
* Update planning for LIKE due to changes in sqlparser (#3194)
* rename array function to make_array (#3199)
* [sqlparser-0.21] Update trimExpr members during planning (#3181)
* Update sqlparser version to use main from git
* Update SqlExpr::Trim struct to match latest sqlparser changes
* use sqlparser 0.21 (#3202)
Co-authored-by: Ayush Dattagupta <ayushdg95@gmail.com>
@ayushdgayushdg mentioned this pull request Aug 22, 2022
MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Dec 1, 2022
* Changes to planning for SHOW TABLES due to changes in sqlparser (apache#3193)
* Update planning for LIKE due to changes in sqlparser (apache#3194)
* rename array function to make_array (apache#3199)
* [sqlparser-0.21] Update trimExpr members during planning (apache#3181)
* Update sqlparser version to use main from git
* Update SqlExpr::Trim struct to match latest sqlparser changes
* use sqlparser 0.21 (apache#3202)
Co-authored-by: Ayush Dattagupta <ayushdg95@gmail.com>
MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Dec 1, 2022
* Changes to planning for SHOW TABLES due to changes in sqlparser (apache#3193)
* Update planning for LIKE due to changes in sqlparser (apache#3194)
* rename array function to make_array (apache#3199)
* [sqlparser-0.21] Update trimExpr members during planning (apache#3181)
* Update sqlparser version to use main from git
* Update SqlExpr::Trim struct to match latest sqlparser changes
* use sqlparser 0.21 (apache#3202)
Co-authored-by: Ayush Dattagupta <ayushdg95@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coreCore DataFusion cratelogical-exprLogical plan and expressionssqlSQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ayushdg@andygrove@codecov-commenter@alamb