Skip to content

fix(conversions): reject a Decimal whose scale is negative - #3997

Open
Rodrigo-Palma wants to merge 1 commit into
apache:mainfrom
Rodrigo-Palma:fix/decimal-negative-scale
Open

Rodrigo-Palma wants to merge 1 commit into
apache:mainfrom
Rodrigo-Palma:fix/decimal-negative-scale

Conversation

@Rodrigo-Palma

Copy link
Copy Markdown

Closes #3996.

to_bytes for DecimalType compared abs(exponent) against the type's scale. A
Decimal carries the negated scale as its exponent, so a value with a negative scale
passed the check as if it had the matching positive one. decimal_to_unscaled then uses
only the digits, so the exponent is dropped and the value is written four orders of
magnitude off, with no error.

t = DecimalType(10, 2)
from_bytes(t, to_bytes(t, Decimal("1E+2")))   # 0.01, expected 100.00

Decimal("100").normalize() is exactly Decimal("1E+2"), so a normalized value, or one
that comes out of arithmetic that trims trailing zeros, hits this.

It matters beyond the round trip: to_bytes writes the lower_bounds and upper_bounds
of a data file (_write_data_file_statistics in pyiceberg/manifest.py) and is used again
in pyiceberg/io/pyarrow.py. A bound written as 0.01 instead of 100.00 makes scan
planning 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 the ValueError the
function 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") against decimal(7, 3) still reports : 4.

Tests

Two cases added to the existing test_datetime_obj_to_bytes parametrization in
tests/test_conversions.py, one using the literal Decimal("1E+2") and one using
Decimal("100").normalize(). Both fail without the source change (DID NOT RAISE).

make lint and make test green locally (4188 passed, 5 skipped).

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

to_bytes silently rescales a Decimal with a negative scale

1 participant