fix: reject decimal promotion that changes the scale#3613
Conversation
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
I've got an ask around the error message, but otherwise looks great.
| @promote.register(DecimalType) | ||
| def _(file_type: DecimalType, read_type: IcebergType) -> IcebergType: | ||
| if isinstance(read_type, DecimalType): | ||
| if file_type.precision <= read_type.precision and file_type.scale == file_type.scale: |
There was a problem hiding this comment.
Oh, this was always true! Well, that's not ideal.
| return read_type | ||
| else: | ||
| raise ResolveError(f"Cannot reduce precision from {file_type} to {read_type}") | ||
| raise ResolveError(f"Cannot promote {file_type} to {read_type}") |
There was a problem hiding this comment.
Can we change this back? The old error message better helped users understand the issue.
The DecimalType handler in promote() guarded scale equality with `file_type.scale == file_type.scale`, which compares the file scale to itself and is always true. As a result any decimal-to-decimal promotion with a widening precision was accepted regardless of the scale. Per the Iceberg spec a decimal may only be promoted when the scale is unchanged and the precision widens (decimal(P, S) to decimal(P2, S) with P2 > P), matching TypeUtil.isPromotionAllowed in the Java implementation. The bug affected both paths that use promote(): on read a differing-scale promotion built a reader at the wrong scale and reinterpreted the stored unscaled integers (silent data corruption), and on write it let a DataFrame column with a different scale pass the compatibility check. Compare the file scale to the read scale instead. The identical tautology in the test oracle masked the defect, so fix it too and add a decimal with a different scale to the promotion matrix, plus explicit regression tests for the schema and read paths. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
A couple nits, but this looks great.
| return True | ||
| if isinstance(file_type, DecimalType) and isinstance(read_type, DecimalType): | ||
| return file_type.precision <= read_type.precision and file_type.scale == file_type.scale | ||
| return file_type.precision <= read_type.precision and file_type.scale == read_type.scale |
There was a problem hiding this comment.
In general, I really don't like how we're handling promotion testing. We copied + pasted this logic and that's how this bug wasn't caught.
nit: Can you have a can_promote_decimal function that has this logic so it's at least shared.
| DoubleType(), | ||
| DecimalType(10, 2), | ||
| DecimalType(100, 2), | ||
| DecimalType(10, 4), |
There was a problem hiding this comment.
We don't really need this. The test you wrote is much more useful.
There's only two things that could possibly happen: an error or an improper promotion. test_promotion didn't recognize an improper error.
2 participants
Rationale for this change
The
DecimalTypehandler inpromote()(pyiceberg/schema.py) guarded scaleequality with
file_type.scale == file_type.scale, which compares the file'sscale to itself and is therefore always true. As a result any decimal-to-decimal
promotion with a widening precision was accepted regardless of whether the scale
matched.
Per the Iceberg spec, a
decimal may only be promoted when the scale is unchanged and the precision widens
(
decimal(P, S)todecimal(P2, S)withP2 > P; "scale cannot change"). Thismatches
TypeUtil.isPromotionAllowedin the Java reference implementation, whichrequires
fromDecimal.scale() == toDecimal.scale()andfromDecimal.precision() <= toDecimal.precision().The defect affected both code paths that call
promote():pyiceberg/avro/resolver.py), a differing-scale promotion was acceptedand a
DecimalReaderwas built at the read scale, reinterpreting the file'sstored unscaled integers at the wrong scale. For example a value stored as
1.23(unscaled123, scale2) would read back as0.0123at scale4.This is silent data corruption rather than an error.
_check_schema_compatibleinpyiceberg/schema.py), a DataFramecolumn of
decimal(9, 2)was accepted as compatible with a table column ofdecimal(18, 4).The fix compares the file scale to the read scale instead. The identical tautology
in the test oracle (
should_promoteintests/test_schema.py) masked the defect,so it is corrected as well.
Are these changes tested?
Yes.
tests/test_schema.py: fixed the mirrored tautology in theshould_promoteoracle, and added
DecimalType(10, 4)toTEST_PRIMITIVE_TYPESso the existingparametrized
test_promotionnow exercises differing-scale pairs (previously alldecimal fixtures were scale 2, so this case was never covered). Added
test_decimal_promotionwith explicit cases: widening precision at fixed scalesucceeds, equal precision/scale resolves, changing the scale raises, and reducing
the precision raises.
tests/avro/test_resolver.py: addedtest_resolve_decimal_to_decimal_change_scalecovering the read path (the data-corruption vector), and updated the existing
reduce-precision assertion to the generalized error message.
Reverting only the source change (keeping the tests) turns the new and
differing-scale cases red with "DID NOT RAISE ResolveError" (the exact corruption
symptom); with the fix the targeted suites pass (388 passed).
make lint(ruff,ruff-format, mypy, uv-lock, license/codespell) passes.
Integration tests that need Docker + Spark were not run in this environment; the
behavior is covered by the unit tests above.
Are there any user-facing changes?
Yes, a behavioral correctness change. A decimal-to-decimal promotion that changes
the scale is now correctly rejected with a
ResolveError(previously it wassilently accepted). This brings PyIceberg in line with the Iceberg spec and the
Java implementation. The error message in the else branch was generalized from
"Cannot reduce precision from {file_type} to {read_type}" to
"Cannot promote {file_type} to {read_type}", since that branch now also fires for
scale changes (matching the wording of the sibling promote handlers).