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.

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::ResourceId with Vec<sqlparser::ast::Ident> in the proof-of-sql crate #449

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

varshith257
Copy link
Contributor

@varshith257 varshith257 commented Dec 30, 2024

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

Rationale for this change

This PR addresses the need to replace the proof_of_sql_parser::ResourceId with the sqlparser::ast::ObjectName in the proof-of-sql crate as part of a larger transition toward integrating the sqlparser .

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 like proof_of_sql_parser::intermediate_ast with sqlparser::ast.

What changes are included in this PR?

  • All instances of proof_of_sql_parser::ResourceId have been replaced with sqlparser::ast::ObjectName
  • A few of them required a ResourceId (e.g. Select etc..), which depends on the ResourceId and will be migrated at the refactoring of Exprs.
  • Every usage of ResourceId has been updated to maintain the original functionality, ensuring no changes to the logic or behavior.
  • The breaking change here is that ObjectName doesn't support Copy trait so we have needed few clones in the places where values are moved

Are these changes tested?

Yes

Part of #235
Fixes #352

@iajoiner iajoiner self-requested a review January 2, 2025 07:53
Copy link
Contributor

@iajoiner iajoiner left a comment

Choose a reason for hiding this comment

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

Really thanks for your PR! I think it will be great if we add some helper function to produce a TableRef from multiple Strings / &strs. That way we can do something like table_ref(["namespace", "table_name"])

crates/proof-of-sql-parser/src/sqlparser.rs Outdated Show resolved Hide resolved
@iajoiner iajoiner marked this pull request as ready for review January 3, 2025 02:42
@varshith257 varshith257 requested a review from iajoiner January 3, 2025 10:15
@varshith257 varshith257 force-pushed the refactor/resourceid branch 6 times, most recently from 3f73eb6 to 5818c99 Compare January 8, 2025 23:56
Copy link
Contributor

@iajoiner iajoiner left a comment

Choose a reason for hiding this comment

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

It is almost there. Please address the comments and I will approve.

@varshith257 varshith257 force-pushed the refactor/resourceid branch 2 times, most recently from ab40d56 to 4aa1cb2 Compare January 9, 2025 21:06
@varshith257 varshith257 force-pushed the refactor/resourceid branch 2 times, most recently from 23d6106 to dfebf2f Compare January 9, 2025 21:32
@varshith257 varshith257 force-pushed the refactor/resourceid branch 2 times, most recently from a40ad3e to e7bbd3a Compare January 9, 2025 21:54
@varshith257 varshith257 requested a review from iajoiner January 9, 2025 21:55
@varshith257 varshith257 force-pushed the refactor/resourceid branch 2 times, most recently from 7948492 to 3eba927 Compare January 9, 2025 22:22
iajoiner
iajoiner previously approved these changes Jan 10, 2025
iajoiner
iajoiner previously approved these changes Jan 13, 2025
@iajoiner iajoiner requested review from tlovell-sxt and removed request for JayWhite2357 January 14, 2025 01:26
@iajoiner iajoiner force-pushed the refactor/resourceid branch from ad271e6 to b9c9454 Compare January 14, 2025 01:26
@varshith257 varshith257 force-pushed the refactor/resourceid branch 3 times, most recently from 4620d11 to 5209eb3 Compare January 15, 2025 15:34
@varshith257 varshith257 changed the title refactor!: proof_of_sql_parser::intermediate_ast::ResourceId with sqlparser::ast::ObjectName in the proof-of-sql crate refactor!: proof_of_sql_parser::intermediate_ast::ResourceId with Vec<sqlparser::ast::Ident> in the proof-of-sql crate Jan 15, 2025
@varshith257 varshith257 requested a review from iajoiner January 15, 2025 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants