Use rust_decimal's builtin PG encode/decode#989
Merged
Conversation
We had enabled the feature on the rust_decimal crate to compile these functions, but were never actually using them. The postgres-types crate already existed as a transitive dependency of rust_decicmal, it's only added to our Cargo.toml so we can use the trait impls that decimal has. Our tests for all the various malformed inputs were helpful here, the only one I think is excessive is rejecting NaN with digits. All the other cases caught places where the decimal crate hit unchecked overflow. It wouldn't have resulted in UB but the fact that it returns nonsense instead of an error is something that's good for us to catch. This does slightly change our behavior, as their encoding implementation fails to remove leading zeroes from the output when the result is an exact power of 10000. This shouldn't matter, as the two values are completely equivalent as far as PG is concerned (and I've verified as such manually). I've updated the test case to reflect this behavior, though I think if we accept this change we should probably just delete it since we're testing a crate's behavior and not our own.
|
|
Contributor
Author
|
@CLAassistant You can't take my rights you don't even have a soul |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
We had enabled the feature on the rust_decimal crate to compile these functions, but were never actually using them. The postgres-types crate already existed as a transitive dependency of rust_decicmal, it's only added to our Cargo.toml so we can use the trait impls that decimal has.
Our tests for all the various malformed inputs were helpful here, the only one I think is excessive is rejecting NaN with digits. All the other cases caught places where the decimal crate hit unchecked overflow. It wouldn't have resulted in UB but the fact that it returns nonsense instead of an error is something that's good for us to catch.
This does slightly change our behavior, as their encoding implementation fails to remove leading zeroes from the output when the result is an exact power of 10000. This shouldn't matter, as the two values are completely equivalent as far as PG is concerned (and I've verified as such manually). I've updated the test case to reflect this behavior, though I think if we accept this change we should probably just delete it since we're testing a crate's behavior and not our own.