From 0375605a976d814d977e153c6786e2b4366a07a1 Mon Sep 17 00:00:00 2001 From: Anas Khan <83116240+anxkhn@users.noreply.github.com> Date: Sat, 4 Jul 2026 19:43:52 +0530 Subject: [PATCH] fix: reject decimal promotion that changes the scale 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> --- pyiceberg/schema.py | 4 ++-- tests/avro/test_resolver.py | 11 ++++++++++- tests/test_schema.py | 16 +++++++++++++++- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/pyiceberg/schema.py b/pyiceberg/schema.py index fd60eb8f94..35e81d37e4 100644 --- a/pyiceberg/schema.py +++ b/pyiceberg/schema.py @@ -1697,10 +1697,10 @@ def _(file_type: BinaryType, read_type: IcebergType) -> IcebergType: @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: + if file_type.precision <= read_type.precision and file_type.scale == read_type.scale: 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}") else: raise ResolveError(f"Cannot promote an decimal to {read_type}") diff --git a/tests/avro/test_resolver.py b/tests/avro/test_resolver.py index f0742e946d..ee4a65f2b0 100644 --- a/tests/avro/test_resolver.py +++ b/tests/avro/test_resolver.py @@ -253,7 +253,16 @@ def test_resolve_decimal_to_decimal_reduce_precision() -> None: with pytest.raises(ResolveError) as exc_info: _ = resolve_reader(DecimalType(19, 25), DecimalType(10, 25)) == DecimalReader(22, 25) - assert "Cannot reduce precision from decimal(19, 25) to decimal(10, 25)" in str(exc_info.value) + assert "Cannot promote decimal(19, 25) to decimal(10, 25)" in str(exc_info.value) + + +def test_resolve_decimal_to_decimal_change_scale() -> None: + # Changing the scale is not a valid promotion, even when the precision widens. + # Allowing it would reinterpret the file's unscaled integers at the wrong scale. + with pytest.raises(ResolveError) as exc_info: + _ = resolve_reader(DecimalType(9, 2), DecimalType(18, 4)) + + assert "Cannot promote decimal(9, 2) to decimal(18, 4)" in str(exc_info.value) def test_column_assignment() -> None: diff --git a/tests/test_schema.py b/tests/test_schema.py index 93ddc16202..3acbb90667 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -68,6 +68,7 @@ DoubleType(), DecimalType(10, 2), DecimalType(100, 2), + DecimalType(10, 4), StringType(), DateType(), TimeType(), @@ -878,7 +879,7 @@ def should_promote(file_type: IcebergType, read_type: IcebergType) -> bool: if isinstance(file_type, BinaryType) and isinstance(read_type, StringType): 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 if isinstance(file_type, FixedType) and isinstance(read_type, UUIDType) and len(file_type) == 16: return True return False @@ -945,6 +946,19 @@ def test_promotion(file_type: IcebergType, read_type: IcebergType) -> None: promote(file_type, read_type) +def test_decimal_promotion() -> None: + # Widening precision while keeping the scale fixed is allowed. + assert promote(DecimalType(9, 2), DecimalType(18, 2)) == DecimalType(18, 2) + # Equal precision and scale resolves to the read type. + assert promote(DecimalType(9, 2), DecimalType(9, 2)) == DecimalType(9, 2) + # Changing the scale is never allowed, even when the precision widens. + with pytest.raises(ResolveError): + promote(DecimalType(9, 2), DecimalType(18, 4)) + # Reducing the precision is not allowed. + with pytest.raises(ResolveError): + promote(DecimalType(18, 2), DecimalType(9, 2)) + + def test_unknown_type_promotion_to_primitive() -> None: """Test that UnknownType can be promoted to primitive types (V3+ behavior)""" unknown_type = UnknownType()