Repository navigation
fix(conversions): reject a Decimal whose scale is negative - #3997
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 pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
Looks great, thanks for doing this!
| # scale (1E+2) must not be read as if it had the matching positive one | ||
| value_scale = -int(exponent) | ||
| if value_scale != primitive_type.scale: | ||
| raise ValueError(f"Cannot serialize value, scale of value does not match type {primitive_type}: {value_scale}") |
There was a problem hiding this comment.
nit: I don't love this error message (decimal scale isn't always a term that people are immediately familiar with), but I think it'll be fine.
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Sorry for the late reply @Rodrigo-Palma, this should have been merged already 👍
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).