fix(conversions): reject a Decimal whose scale is negative - #3997
Open
Rodrigo-Palma wants to merge 1 commit into
Open
Rodrigo-Palma wants to merge 1 commit into
Rodrigo-Palma wants to merge 1 commit into
Conversation
to_bytes compared abs(exponent) against the type scale, so a Decimal carrying
a negative scale passed as if it had the matching positive one. The exponent is
then dropped by decimal_to_unscaled, and the value is written four orders of
magnitude off without an error: Decimal('1E+2') stored in a decimal(10, 2)
reads back as 0.01.
Decimal('100').normalize() produces exactly that form, and to_bytes writes the
lower and upper bounds of a data file, so a wrong bound prunes files that do
hold matching rows.
Compare the signed scale instead.
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
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.
Closes #3996.
to_bytesforDecimalTypecomparedabs(exponent)against the type's scale. ADecimalcarries the negated scale as its exponent, so a value with a negative scalepassed the check as if it had the matching positive one.
decimal_to_unscaledthen usesonly the digits, so the exponent is dropped and the value is written four orders of
magnitude off, with no error.
Decimal("100").normalize()is exactlyDecimal("1E+2"), so a normalized value, or onethat comes out of arithmetic that trims trailing zeros, hits this.
It matters beyond the round trip:
to_byteswrites thelower_boundsandupper_boundsof a data file (
_write_data_file_statisticsinpyiceberg/manifest.py) and is used againin
pyiceberg/io/pyarrow.py. A bound written as0.01instead of100.00makes scanplanning prune files that do hold matching rows, so a query returns fewer rows without
reporting anything.
Change
Compare the signed scale,
-exponent, so a mismatching value raises theValueErrorthefunction already raises for any other scale mismatch. Rescaling to the type's scale was the
other option, but that widens the contract of a function that today requires an exact
match, so this keeps the existing one.
Values with a positive scale are unaffected, and the error message is unchanged for them:
Decimal("123.4567")againstdecimal(7, 3)still reports: 4.Tests
Two cases added to the existing
test_datetime_obj_to_bytesparametrization intests/test_conversions.py, one using the literalDecimal("1E+2")and one usingDecimal("100").normalize(). Both fail without the source change (DID NOT RAISE).make lintandmake testgreen locally (4188 passed, 5 skipped).