moomindani commented on code in PR #3630:
URL: https://github.com/apache/iceberg-python/pull/3630#discussion_r3827827424
##########
pyiceberg/partitioning.py:
##########
@@ -115,13 +117,34 @@ def map_source_ids_onto_source_id(cls, data: Any) -> Any:
if len(source_ids) == 0:
raise ValueError("Empty source-ids is not allowed")
if len(source_ids) > 1:
- raise ValueError("Multi argument transforms are not
yet supported")
+ if data.get("transform") is None:
Review Comment:
Good catch on both counts.
You're right that the block is skipped when both keys are present — the
guard was `"source-id" not in data and "source-ids" in data`. The consequence
is worse than just losing the normalization: a multi-argument field written
with both keys keeps its real transform instead of being replaced with
`UnknownTransform`, so we would evaluate e.g. `bucket[4]` against only the
first source column rather than treating it as unknown.
The spec only ever writes one of the two keys ("For partition fields with a
transform with a single argument, only `source-id` is written. In case of a
multi-argument transform, only `source-ids` is written." — same wording for
sort fields), so both-present is non-conformant input. I went with the lenient
read: `source-ids` is authoritative whenever it is present, and a `source-id`
next to it is ignored. That also means an empty `source-ids` is now rejected
even when a `source-id` is present. There is no Java implementation of
`source-ids` to align with — the string does not appear anywhere in
apache/iceberg's Java tree — so the spec text is the only reference here.
The nesting is gone as well: the validator is now a flat sequence of guards
with early returns. New tests cover the both-present case for multi-argument,
single-element and empty `source-ids`, on partition fields and sort fields.
##########
pyiceberg/partitioning.py:
##########
@@ -77,6 +78,7 @@ class PartitionField(IcebergBaseModel):
"""
source_id: int = Field(alias="source-id")
+ source_ids: list[int] | None = Field(alias="source-ids", default=None,
repr=False)
Review Comment:
Done, and this was the right call — the same three pieces (before-validator,
model serializer, `__str__`) were duplicated between `PartitionField` and
`SortField`.
Added `TransformSourceMixin` in `pyiceberg/transforms.py`, which both
classes now inherit. It owns the `source-id`/`source-ids` fields, the
validator, the serializer, and two accessors:
- `transform_arguments -> list[int]` — the source column ids the transform
is applied to (`source_ids` when multi-argument, otherwise `[source_id]`)
- `is_multi_argument -> bool` — derived from the above, so the arity check
has a single definition
`__str__` in both classes is now a single line over `transform_arguments`,
and the only code that touches the raw `"source-id"` / `"source-ids"` keys is
the mixin. Both files shrank by 46 lines.
It lives in `transforms.py` because `UnknownTransform` is already there and
both call sites import from it, so this needed no new module and no new
dependency edge. Happy to move it, or rename the accessors, if you would rather
have it elsewhere.
--
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]
