sungwy commented on code in PR #3529:
URL: https://github.com/apache/iceberg-python/pull/3529#discussion_r3580646320
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1970,13 +1970,14 @@ def _cast_if_needed(self, field: NestedField, values:
pa.Array) -> pa.Array:
return values.cast(target_type)
raise ValueError(f"Unsupported schema projection from
{values.type} to {target_type}")
elif isinstance(field.field_type, (IntegerType, LongType)):
- # Cast smaller integer types to target type for
cross-platform compatibility
- # Only allow widening conversions (smaller bit width to
larger)
- # Narrowing conversions fall through to promote() handling
below
+ # Cast integer types for cross-platform compatibility
(e.g. Spark reads):
+ # widening (smaller bit width to larger) and
unsigned-to-signed at same width
if pa.types.is_integer(values.type):
source_width = values.type.bit_width
target_width = target_type.bit_width
- if source_width < target_width:
+ if source_width < target_width or (
+ pa.types.is_unsigned_integer(values.type) and
source_width <= target_width
Review Comment:
Thanks again for digging into this @JeroenSchmidt.
I thought about this a bit more, and taking a step back: the root issue is
that we're writing non-conformant Parquet.
[Table Spec: Appendix A](https://iceberg.apache.org/spec/#parquet), which
should be our source of truth, maps `int`/`long` to signed `int32`/`int64` with
no unsigned annotation, so the `UINT_32` we emit today as a result of
Arrow→Iceberg conversion is already out of spec. This PR fixes that symptom,
but I think the cleaner fix is one step earlier.
There are really two steps on the write path: (1) the Arrow→Iceberg mapping
picks the Iceberg type for a `uint32` column, then (2) the write casts the data
to match it. Today step 1 (`_ConvertToIceberg.primitive`) keys only on bit
width, so `uint32` maps to `int` (int32). IMHO that's where the loss is locked
in, since int32 can't hold `uint32` values ≥ 2^31. This PR's same-width cast is
then left narrowing into that a type that's already too small.
If step 1 instead mapped `uint32` → `long`, step 2 becomes a plain lossless
widening (`uint32 → int64`), it's spec-conformant, and the same-width cast
isn't needed at all. Same idea for `uint8`/`uint16` → `int` (already lossless).
The only cases with no lossless target are `uint64` (no Iceberg type wider
than int64) and a `uint32` appended to an already-declared `int` column. There
I'd rather reject the write with a clear error than silently cast it to its
signed counterpart. A lossy conversion should be the caller's explicit choice,
not something we do quietly.
So I'm hesitant to land the same-width cast as-is. Could we instead (1) map
`uint32 → long` and (2) reject unsigned writes with no lossless target?
--
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]
