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: remove intermediate decimal and move decimal into PoSQL parser crate #113

Open
7 tasks
Dustin-Ray opened this issue Aug 16, 2024 · 5 comments
Open
7 tasks
Labels
good first issue Good for newcomers refactor Code cleanup or reorganization

Comments

@Dustin-Ray
Copy link
Contributor

Dustin-Ray commented Aug 16, 2024

Background and Motivation

Currently, the code handling decimal parsing and conversions is somewhat scattered. This issue seeks to resolve this by using the bigdecimal::BigDecimal type for as much as possible.

Important Note: the BigDecimal type does not trim leading and trailing zeros. Care should be taken when dealing with this.

Requested Changes

To resolve this issue, we should:

  • Add a BigDecimalExt extension trait in the proof_of_sql::base::math module. It should have, at minimum,
    • precision and scale methods akin to the existing IntermediateDecimal::precision and IntermediateDecimal::scale methods. NOTE: the precision and scale should be the values after trimming any leading and trailing zeros. This is what the current implementation (implicitly) does.
    • a try_into_scalar_with_precision_and_scale method akin to the exiting try_into_to_scalar function.
  • Eliminate the proof_of_sql_parser::intermediate_decimal::IntermediateDecimal and proof_of_sql::base::math::decimal::Decimal types, in favor of using bigdecimal::BigDecimal along with the BigDecimalExt extension trait.

Optional:

  • Other refactoring and/or cleanup is welcome, but not immediately clear to me what should be done. These likely should be done in a follow up PR/issue. Some possibilities are:
    • It is not clear if the DecimalError enum is needed or if bigdecimal::ParseBigDecimalError or similar is sufficient.
    • The scale_scalar function should likely be moved into the proof_of_sql::base::scalar module.
@Dustin-Ray Dustin-Ray added good first issue Good for newcomers and removed good first issue Good for newcomers labels Sep 7, 2024
@JayWhite2357 JayWhite2357 added the good first issue Good for newcomers label Sep 13, 2024
Copy link

algora-pbc bot commented Sep 25, 2024

💎 $100 bounty • Space and Time

Steps to solve:

  1. Start working: (Optional) Comment /attempt #113 with your implementation plan. Note: we will only assign an issue if you include an implementation plan with a time estimate. Additionally, to be assigned an issue, you must have previously contributed to the project. You can still work on an issue and submit a PR without being assigned.
  2. Submit work: Create a pull request including /claim #113 in the PR body to claim the bounty
  3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

Thank you for contributing to spaceandtimelabs/sxt-proof-of-sql!

Add a bountyShare on socials

Attempt Started (GMT+0) Solution
🔴 @onyedikachi-david Sep 27, 2024, 1:25:00 PM WIP
🟢 @Bhavyajain21 Sep 28, 2024, 4:16:59 PM #269
🟢 @pulkitgovrani #196

@onyedikachi-david
Copy link

onyedikachi-david commented Sep 27, 2024

/attempt #113

Algora profile Completed bounties Tech Active attempts Options
@onyedikachi-david 7 bounties from 4 projects
JavaScript, Shell
﹟168, ﹟163
Cancel attempt

@Bhavyajain21
Copy link

Bhavyajain21 commented Sep 28, 2024

/attempt #113

Algora profile Completed bounties Tech Active attempts Options
@Bhavyajain21 15 bounties from 3 projects
TypeScript, Rust,
JavaScript & more
Cancel attempt

@JayWhite2357 JayWhite2357 added enhancement New feature or request refactor Code cleanup or reorganization and removed enhancement New feature or request labels Oct 8, 2024
@JayWhite2357
Copy link
Contributor

This is blocking me at the moment, and none of the active PRs are passing CI or appear to be close enough to complete this quickly enough. I am closing the bounty in favor of #283.

@algora-pbc algora-pbc bot removed the 💎 Bounty label Oct 20, 2024
@hp77-creator
Copy link

Hi @JayWhite2357 , this issue is resolved, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers refactor Code cleanup or reorganization
Projects
None yet
Development

No branches or pull requests

5 participants