akubera / bigdecimal-rs

Arbitrary precision decimal crate for Rust
Other
284 stars 71 forks source link

Add `get_scale` method to `BigDecimal` for efficient number type determination #116

Closed zmrdltl closed 1 year ago

zmrdltl commented 1 year ago

Context

When utilizing the bigdecimal within the SQL parser, specifically in the context of gluesql, there's a desire to identify "1.0" as a float rather than an integer. related PR

The existing mechanism for determining the type was:

fn is_integer_representation(bigdecimal_val: &BigDecimal) -> bool {
  let bigdecimal_val_str = bigdecimal_val.abs().to_string();
  bigdecimal_val.abs().digits() == bigdecimal_val_str.len() as u64
}

This method, which converts the number to a string to determine its length, introduces unnecessary overhead and can be deemed inefficient.

Solution

Introducing the get_scale method allows direct access to the scale of a BigDecimal, enabling users to efficiently determine if a number is an integer or has a fractional component. This change does not modify the existing is_number() function, which currently classifies "1.0" as an integer.

Benefits

By providing direct access to the scale, users who wish to treat "1.0" as a float can easily implement this behavior. This update enhances the flexibility and efficiency for all users of the library.

Note

Care has been taken to ensure backwards compatibility and adherence to the library's design principles.

akubera commented 1 year ago

~Was the is_integer() method not sufficient here?~ Reread - I see you want to check if precision extends beyond the decimal point (even zero).

Regardless, get_scale is present in the standard Java BigDecimal (though just called scale; I prefer get_scale as "scale" could be interpreted as a verb.)

Alternatively, Python and Ruby don't have scale but exponent(), which I think makes more sense to humans. We could do both (n.get_exponent() == -n.get_scale()), but we have to acknowledge there could be an overflow error in such a conversion.

zmrdltl commented 1 year ago

~Was the is_integer() method not sufficient here?~ Reread - I see you want to check if precision extends beyond the decimal point (even zero).

Regardless, get_scale is present in the standard Java BigDecimal (though just called scale; I prefer get_scale as "scale" could be interpreted as a verb.)

Alternatively, Python and Ruby don't have scale but exponent(), which I think makes more sense to humans. We could do both (n.get_exponent() == -n.get_scale()), but we have to acknowledge there could be an overflow error in such a conversion.

Thank you for the constructive feedback. It allowed me to reconsider the naming of the function. Based on the Rust naming conventions, it's recommended not to use the "get" prefix for getters, which makes me lean towards renaming the function for better alignment.

The name "exponent" seems ambiguous when the function is intended to access the scale of the BigDecimal class and return its value. I would suggest a more descriptive name like decimal_scale() or fractional_digits() to more accurately convey its purpose.

akubera commented 1 year ago

fractional_digit_count()?

zmrdltl commented 1 year ago

that will be nice :) thank u for ur suggestion

codecov-commenter commented 1 year ago

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (656f335) 83.72% compared to head (88cee65) 76.10%.

:exclamation: Current head 88cee65 differs from pull request most recent head 60a44c1. Consider uploading reports for the commit 60a44c1 to get more accurate results

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## trunk #116 +/- ## ========================================== - Coverage 83.72% 76.10% -7.62% ========================================== Files 22 11 -11 Lines 2249 2030 -219 Branches 2249 2030 -219 ========================================== - Hits 1883 1545 -338 + Misses 97 87 -10 - Partials 269 398 +129 ``` | [Files](https://app.codecov.io/gh/akubera/bigdecimal-rs/pull/116?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Kubera) | Coverage Δ | | |---|---|---| | [src/lib.rs](https://app.codecov.io/gh/akubera/bigdecimal-rs/pull/116?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Kubera#diff-c3JjL2xpYi5ycw==) | `72.40% <45.45%> (+0.60%)` | :arrow_up: | ... and [13 files with indirect coverage changes](https://app.codecov.io/gh/akubera/bigdecimal-rs/pull/116/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Kubera)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.