Repository navigation
fix(conversions): reject a Decimal whose scale is negative - #3997
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 👍
DecimalLiteral.to and StringLiteral.to compared abs(exponent) against the
type's scale, so a filter value like Decimal("1E+2") bound to decimal(10, 2)
as if it were 100.00 while keeping unscaled value 1. Bucket and truncate
projections then pruned the files holding matching rows and the scan silently
returned fewer rows. Compare the signed scale so binding raises instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM
I pushed a commit (c796d23) to fix a similar issue in pyiceberg/expressions/literals.py and add tests for error message
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.
The same
abs(exponent)comparison is inDecimalLiteral.toandStringLiteral.to, so afilter value like
Decimal("1E+2")binds todecimal(10, 2)as if it were100.00whilekeeping unscaled value
1. Bucket and truncate projections then prune the files that holdmatching rows: on a
bucket(16, d)table,EqualTo("d", Decimal("1E+2"))returns 0 rowswhere
Decimal("100.00")returns 1.Change
Compare the signed scale,
-exponent, into_bytes,DecimalLiteral.toandStringLiteral.to, so a mismatching value raises theValueErroreach already raises forany 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_raise_on_incorrect_precision_or_scaleparametrization intests/test_conversions.py, one using the literalDecimal("1E+2")and one usingDecimal("100").normalize(). Both fail without the source change (DID NOT RAISE).One case each added to
test_decimal_to_decimal_conversionandtest_string_to_decimal_type_invalid_valueintests/expressions/test_literals.py, bothfailing without the source change.
make lintandmake testgreen locally (4188 passed, 5 skipped).