refactor!: remove IntermediateDecimal in favor of BigDecimal
#283
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 20, 2024
JayWhite2357
force-pushed
the
refactor/remove-intermediate-decimal
branch
3 times, most recently
from
3f46e97
to
2eb52a4
Compare
iajoiner
approved these changes
Oct 20, 2024
`BigDecimal` gives "precision" and "scale" as 64-bit integers. `IntermediateDecimal` previously simply converted these to 8-bit integers, meaning that for precision or scale of 256 or 128 digits the code will return the wrong value. (Until recently, it actually wrapped the value, which is even more problematic.) This commit fixes this by returning the proper value and requiring upstream usage to handle any casting explicitly as needed. To facilitate this, a conversion from `u64` to `Precision` is added.
…ermediateDecimal Implementing `TryFrom<&str>` and `TryFrom<String>` is an anti-pattern since this is exactly what `FromStr` is for. This commit removes the implementations and any uses of them.
… crate precision and scale are unused within the `proof-of-sql-parser` crate, and as a result, it doesn't make sense to have that logic in the crate. This commit moves it into the `proof-of-sql` crate via a `IntermediateDecimalExt` trait. NOTE: this trait will be renamed to `BigDecimalExt` in a future commit.
…ermediate_decimal_to_scalar` `try_into_to_scalar` is poorly named. Even `try_convert_intermediate_decimal_to_scalar` is not great. This will be further renamed in another commit, but renaming it here helps clarify future code changes.
The only place within the intermediate AST that uses `IntermediateDecimal` is the `Literal`. We will replace it in the `Literal` itself in the next commit. This replaces it with `BigDecimal` in the utility function.
This actually does the replacement of `IntermediateDecimal` with `BigDecimal`. All of the previous commits in this PR set up for this commit, which makes this commit very straightforward. NOTE: care is taken to ensure that trailing and leading zeros are removed when needed. The following commits will be cleanup.
This error type will be tidied up in a future PR, but it is at least moved out of the `proof-of-sql-parser` crate. Fixing errors types is a bit more complex, so to keep this PR small, we opt to only do this here, even though it is not tidy.
As this extension trait now applied to `BigDecimal`, it is named appropriately.
This is an unrelated commit. The `Decimal` type is unused. We remove it here. If it is needed later, it can be added back.
iajoiner
force-pushed
the
refactor/remove-intermediate-decimal
branch
from
2eb52a4
to
73077df
Compare
|
🎉 This PR is included in version 0.32.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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Currently, the code handling decimal parsing and conversions is somewhat scattered. This issue seeks to resolve this by using the
bigdecimal::BigDecimaltype for as much as possible.What changes are included in this PR?
Remove
IntermediateDecimalin favor ofBigDecimalPlease read the individual commit messages for detailed changes.
Are these changes tested?
Yes.