-
Notifications
You must be signed in to change notification settings - Fork 527
fix: reject decimal promotion that changes the scale #3613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,7 @@ | |
| DoubleType(), | ||
| DecimalType(10, 2), | ||
| DecimalType(100, 2), | ||
| DecimalType(10, 4), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| 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() | ||
|
|
||
There was a problem hiding this comment.
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.