rambleraptor commented on code in PR #3613:
URL: https://github.com/apache/iceberg-python/pull/3613#discussion_r3547427918
##########
tests/test_schema.py:
##########
@@ -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
Review 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.
##########
tests/test_schema.py:
##########
@@ -68,6 +68,7 @@
DoubleType(),
DecimalType(10, 2),
DecimalType(100, 2),
+ DecimalType(10, 4),
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
