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

feat: add AddSubtractExpr #14

Merged
merged 3 commits into from
Jun 21, 2024
Merged

feat: add AddSubtractExpr #14

merged 3 commits into from
Jun 21, 2024

Conversation

iajoiner
Copy link
Contributor

@iajoiner iajoiner commented Jun 14, 2024

Rationale for this change

We need to add support for arithmetic. Let's start from + and -.

What changes are included in this PR?

  • Add support for + and - in proof-of-sql crate
  • generalize scale_and_subtract_eval to scale_and_add_subtract_eval
  • make sure we pass in scales in any operation that can involve decimals

Are these changes tested?

Yes.

@iajoiner iajoiner changed the title feat: add AddExpr & SubExpr feat: add AddSubtractExpr Jun 14, 2024
@iajoiner iajoiner force-pushed the feat/add-sub branch 4 times, most recently from f2133e5 to f38ae8d Compare June 14, 2024 23:25
@iajoiner iajoiner force-pushed the feat/add-sub branch 3 times, most recently from 6171344 to ed54c25 Compare June 16, 2024 06:51
@iajoiner iajoiner marked this pull request as ready for review June 16, 2024 06:51
@iajoiner iajoiner force-pushed the feat/add-sub branch 4 times, most recently from 8c2a74c to 55e22a4 Compare June 18, 2024 06:35
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.

Please add some tests that deal with overflow. I'm concerned that it isn't handled properly.

@JayWhite2357
Copy link
Contributor

JayWhite2357 commented Jun 20, 2024

See #20 (in particular, the and_expr_test.rs file) for how I think we should have the tests look going forward.

@iajoiner iajoiner force-pushed the feat/add-sub branch 2 times, most recently from 025cd19 to 5dd3605 Compare June 20, 2024 17:47
@iajoiner iajoiner force-pushed the feat/add-sub branch 6 times, most recently from 9c23c26 to 75a4b23 Compare June 20, 2024 19:56
JayWhite2357
JayWhite2357 previously approved these changes Jun 21, 2024
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. One NIT.

crates/proof-of-sql/src/sql/ast/provable_expr_plan.rs Outdated Show resolved Hide resolved
JayWhite2357
JayWhite2357 previously approved these changes Jun 21, 2024
- add `AddSubExpr` and enable + and - elsewhere
- generalize `scale_and_subtract_eval` to `scale_and_add_subtract_eval`
- make sure we pass in scales in any operation that can involve decimals
@iajoiner iajoiner merged commit 37f93f6 into main Jun 21, 2024
8 checks passed
@iajoiner iajoiner deleted the feat/add-sub branch June 21, 2024 22:54
Copy link

🎉 This PR is included in version 0.2.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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants