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::BinaryOp with sqlparser::ast::BinaryOp in the proof-of-sql crate #362

Merged
merged 1 commit into from
Nov 17, 2024

Conversation

varshith257
Copy link
Contributor

@varshith257 varshith257 commented Nov 9, 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::intermediate_ast::BinaryOp with the sqlparser::ast::BinaryOp 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::intermediate_ast::BinaryOp have been replaced with sqlparser::ast::BinaryOp
  • Every usage of BianryOp has been updated to maintain the original functionality, ensuring no changes to the logic or behavior.
  • Any unsupported BinaryOp variants from sqlparser have been appropriately handled using existing error handling mechanisms (i.e., the Unsupported variant in ExpressionEvaluationError).

Are these changes tested?

Yes

Closes #349
Part of #235

@varshith257
Copy link
Contributor Author

@JayWhite2357 Can I add ignore clippy to this?
https://github.com/spaceandtimelabs/sxt-proof-of-sql/actions/runs/11756005825/job/32751686248#step:6:155

If we go with clippy the signature of impl -> BinaryOP in sqlparser.rs need to get changed?

Which approach could be the best here?

@iajoiner
Copy link
Contributor

@varshith257 If you think clippy is being really absurd you can add ignores.

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.

Please rebase with main

@varshith257 varshith257 force-pushed the refactor-binaryop branch 3 times, most recently from 144a86f to eb70802 Compare November 12, 2024 07:32
@varshith257 varshith257 changed the title refactor!: PoSQLBinaryOP to use sqlparser::ast::BinaryOP refactor!: proof_of_sql_parser::intermediate_ast::BinaryOp with sqlparser::ast::BinaryOp in the proof-of-sql crate Nov 12, 2024
@varshith257 varshith257 changed the title refactor!: proof_of_sql_parser::intermediate_ast::BinaryOp with sqlparser::ast::BinaryOp in the proof-of-sql crate refactor!: proof_of_sql_parser::intermediate_ast::BinaryOp with sqlparser::ast::BinaryOp in the proof-of-sql crate Nov 12, 2024
@varshith257 varshith257 force-pushed the refactor-binaryop branch 2 times, most recently from db21763 to 37c1e84 Compare November 12, 2024 19:22
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.

Looks great! Can we pass in references to not trigger clippy::needless_pass_by_value?

@varshith257
Copy link
Contributor Author

@iajoiner This is the conflict #362 (comment)

@iajoiner
Copy link
Contributor

@varshith257 OK. Actually in this case I think it is better to comply. Normally it shouldn't hurt to add a few &. If not please let me know.

@iajoiner iajoiner self-requested a review November 12, 2024 20:17
iajoiner
iajoiner previously approved these changes Nov 12, 2024
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! This is awesome!
P.S. I don't really think we need to make changes in this PR to satisfy clippy::needless_pass_by_value since we pass in UnaryOp by value as well and clippy doesn't flag that.

Copy link
Contributor

@JayWhite2357 JayWhite2357 left a comment

Choose a reason for hiding this comment

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

Looks good. Only request is the clippy lint.

crates/proof-of-sql/src/sql/parse/query_context_builder.rs Outdated Show resolved Hide resolved
@JayWhite2357
Copy link
Contributor

JayWhite2357 commented Nov 13, 2024

@JayWhite2357 Can I add ignore clippy to this? https://github.com/spaceandtimelabs/sxt-proof-of-sql/actions/runs/11756005825/job/32751686248#step:6:155

If we go with clippy the signature of impl -> BinaryOP in sqlparser.rs need to get changed?

Which approach could be the best here?

You shouldn't need to change sqlparser.rs. You should just need to add a few &s where clippy asks, and then add a few more at the locations calling those functions. This lint it generally quite simply to resolve unless the method is being called a lot, which I don't think it is here.

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.

Thanks for the fixes!

@iajoiner iajoiner enabled auto-merge November 17, 2024 12:35
@iajoiner iajoiner merged commit 2b31823 into spaceandtimelabs:main Nov 17, 2024
11 checks passed
Copy link

🎉 This PR is included in version 0.47.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants