-
Notifications
You must be signed in to change notification settings - Fork 126
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
refactor!: proof_of_sql_parser::intermediate_ast::Identifier
with sqlparser::ast::Ident
in the proof-of-sql crate
#382
Conversation
56bd993
to
216f1dc
Compare
Head branch was pushed to by a user without write access
f3a774a
to
806ffd9
Compare
7e39f30
to
7b485b3
Compare
7b485b3
to
1863ff2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@varshith257 Really thanks for your hard work! This is mostly good.
One comment:
Please remove changes to the examples. Changes in the rest of the code base should be sufficient in getting them to work.
crates/proof-of-sql/src/base/arrow/owned_and_arrow_conversions.rs
Outdated
Show resolved
Hide resolved
bee2ad2
to
db13153
Compare
d848826
to
044b954
Compare
0be1a68
to
5920731
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR!
Two requests, and then I'll merge it.
- Don't normalize the identifier names. Leave them as is.
- Don't panic when converting Ident to Identifier. Instead return an Error. Not every valid Ident is a valid Identifier.
crates/proof-of-sql/src/base/database/owned_table_test_accessor.rs
Outdated
Show resolved
Hide resolved
crates/proof-of-sql/src/sql/postprocessing/group_by_postprocessing.rs
Outdated
Show resolved
Hide resolved
crates/proof-of-sql/src/sql/postprocessing/group_by_postprocessing.rs
Outdated
Show resolved
Hide resolved
bc85e1f
to
0f69d0f
Compare
@JayWhite2357 In terms of WASM sizes I think we can proceed with the changes. |
…parser::ast::Ident in the proof-of-sql crate
22bbcfc
to
179da05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@varshith257 Thanks again for your hard work!
🎉 This PR is included in version 0.63.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Please be sure to look over the pull request guidelines here: https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.
Please go through the following checklist
!
is used if and only if at least one breaking change has been introduced.source scripts/run_ci_checks.sh
.Rationale for this change
This PR addresses the need to replace the
proof_of_sql_parser::Identifier
with thesqlparser::ast::Ident
in theproof-of-sql
crate as part of a larger transition toward integrating thesqlparser
.This change is a subtask of issue #235, with the main goal of streamlining the repository by switching to the
sqlparser
crate and gradually replacing intermediary constructs likeproof_of_sql_parser::intermediate_ast
withsqlparser::ast
.What changes are included in this PR?
All instances of
proof_of_sql_parser::Identifier
have been replaced withsqlparser::ast::Ident
A few of them required an identifier (e.g. Expression::Column, etc..), which is dependent on the Identifier and will be migrated at the refactoring of Exprs.
Every usage of
Identifier
has been updated to maintain the original functionality, ensuring no changes to the logic or behavior.The breaking change here is that
Ident
doesn't supportCopy
trait so we have needed the clones in the places where values are movedDeleted the test
we_cannot_convert_a_record_batch_if_it_has_repeated_column_names
because thesqlparser
now differentiates between uppercase and lowercase identifiers. Case normalization is no longer applied andsqlparser
treatsa
andA
as distinct identifiers.Examples are updated to align with
sqlparser
's case-sensitive behavior.Are these changes tested?
Yes
Part of #235