anxkhn opened a new pull request, #3613: URL: https://github.com/apache/iceberg-python/pull/3613
# Rationale for this change The `DecimalType` handler in `promote()` (`pyiceberg/schema.py`) guarded scale equality with `file_type.scale == file_type.scale`, which compares the file's scale 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](https://iceberg.apache.org/spec/#schema-evolution), 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`; "scale cannot change"). This matches `TypeUtil.isPromotionAllowed` in the Java reference implementation, which requires `fromDecimal.scale() == toDecimal.scale()` and `fromDecimal.precision() <= toDecimal.precision()`. The defect affected both code paths that call `promote()`: - On read (`pyiceberg/avro/resolver.py`), a differing-scale promotion was accepted and a `DecimalReader` was built at the read scale, reinterpreting the file's stored unscaled integers at the wrong scale. For example a value stored as `1.23` (unscaled `123`, scale `2`) would read back as `0.0123` at scale `4`. This is silent data corruption rather than an error. - On write (`_check_schema_compatible` in `pyiceberg/schema.py`), a DataFrame column of `decimal(9, 2)` was accepted as compatible with a table column of `decimal(18, 4)`. The fix compares the file scale to the read scale instead. The identical tautology in the test oracle (`should_promote` in `tests/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 the `should_promote` oracle, and added `DecimalType(10, 4)` to `TEST_PRIMITIVE_TYPES` so the existing parametrized `test_promotion` now exercises differing-scale pairs (previously all decimal fixtures were scale 2, so this case was never covered). Added `test_decimal_promotion` with explicit cases: widening precision at fixed scale succeeds, equal precision/scale resolves, changing the scale raises, and reducing the precision raises. - `tests/avro/test_resolver.py`: added `test_resolve_decimal_to_decimal_change_scale` covering 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 was silently 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). <!-- If a maintainer wants this noted in the changelog, the `changelog` label cannot be set by an external contributor. --> -- 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]
