Skip to content

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

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.

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

Merged
merged 2 commits into from
Aug 19, 2022

Conversation

ayushdg
Copy link
Contributor

@ayushdg ayushdg commented Aug 16, 2022

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-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner labels Aug 16, 2022
@andygrove
Copy link
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

@andygrove andygrove mentioned this pull request Aug 18, 2022
5 tasks
@ayushdg ayushdg changed the base branch from master to sqlparser-0.21 August 18, 2022 17:49
@ayushdg ayushdg marked this pull request as ready for review August 18, 2022 17:50
@ayushdg ayushdg changed the title [WIP] Update trimExpr members during planning Update trimExpr members during planning Aug 18, 2022
@andygrove andygrove changed the title Update trimExpr members during planning [sqlparser-0.21] Update trimExpr members during planning Aug 18, 2022
@andygrove
Copy link
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 ayushdg force-pushed the update-trimexpr-sqlparser branch from bd827f6 to 1e46ff8 Compare August 18, 2022 23:17
@ayushdg
Copy link
Contributor Author

ayushdg commented Aug 18, 2022

@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

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 Files Coverage Δ
datafusion/sql/src/planner.rs 80.61% <70.00%> (-0.14%) ⬇️
datafusion/expr/src/logical_plan/plan.rs 77.95% <0.00%> (+0.34%) ⬆️

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

Copy link
Member

@andygrove andygrove left a comment

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
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.21 Aug 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>
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants