Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyiceberg/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -1697,7 +1697,7 @@ 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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this was always true! Well, that's not ideal.

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}")
Expand Down
9 changes: 9 additions & 0 deletions tests/avro/test_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,15 @@ def test_resolve_decimal_to_decimal_reduce_precision() -> None:
assert "Cannot reduce precision from 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 reduce precision from decimal(9, 2) to decimal(18, 4)" in str(exc_info.value)


def test_column_assignment() -> None:
int_schema = {
"type": "record",
Expand Down
16 changes: 15 additions & 1 deletion tests/test_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
DoubleType(),
DecimalType(10, 2),
DecimalType(100, 2),
DecimalType(10, 4),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

StringType(),
DateType(),
TimeType(),
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if isinstance(file_type, FixedType) and isinstance(read_type, UUIDType) and len(file_type) == 16:
return True
return False
Expand Down Expand Up @@ -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()
Expand Down