You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Fokko (via GitHub)" <gi...@apache.org> on 2025/02/03 06:38:30 UTC
[PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Fokko opened a new pull request, #12161:
URL: https://github.com/apache/iceberg/pull/12161
I was looking at adding support for `source-ids` in PyIceberg, but noticed that it was also still lacking for Java.
I've noticed that `source-ids` are also backported to V1 and V2 tables, which surprised me since this might break existing V2 implementations that are unaware of the `source-ids`.
This PR reconsiders https://github.com/apache/iceberg/pull/9661
And more specifically: https://lists.apache.org/thread/9opgkrpqhzp3nl8hdohgnk1m1zxnxmq0
It would be good to only allow multi-arg transforms from V3 onwards, and avoid having some implementations support this by setting a flag. Other implementations might not be aware of this implementation and drop the 2nd argument onward:
```json
{
"source-id": 19,
"source-ids": [19, 25],
"field-id": 1000,
"name": "ts_bucket",
"transform": "bucket"
}
```
The V2 implementation that is unaware of the `source-ids` (PyIceberg, Iceberg-Rust and others), would produce:
```json
{
"source-id": 19,
"field-id": 1000,
"name": "ts_bucket",
"transform": "bucket"
}
```
Breaking the partitioning silently 😱
cc @rdblue @szehon-ho @advancedxy @jbonofre
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12161:
URL: https://github.com/apache/iceberg/pull/12161#discussion_r1939350765
##########
format/spec.md:
##########
@@ -1379,7 +1379,7 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
| required | required | omitted | **`source-id`** | `JSON int` | 1 |
-| optional | optional | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
+| | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
Review Comment:
+1
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #12161:
URL: https://github.com/apache/iceberg/pull/12161
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #12161:
URL: https://github.com/apache/iceberg/pull/12161#issuecomment-2630102760
@Fokko Thanks ! As discussed together, I was about to propose this, so totally agree !
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #12161:
URL: https://github.com/apache/iceberg/pull/12161#issuecomment-2635591315
@Fokko Thanks for bring this up. I'm supporting this to only allow multi-arg transforms for V3 onwards. When multi-arg transform was first added to the spec, V3 was far from completion and we want to make it possible for V2 to support that. I think it's good timing to reconsider that as the multi-arg transform is still lacking in the java side.
> Other implementations might not be aware of this implementation and drop the 2nd argument onward:
To clarify, this might not happen. All the existing transform should be the singe-arg transform. For multi-arg transforms, it shall be added to the spec first and other implementation should be aware of the new transforms if it's spec compliant.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12161:
URL: https://github.com/apache/iceberg/pull/12161#discussion_r1938866471
##########
format/spec.md:
##########
@@ -1400,7 +1400,7 @@ In some cases partition specs are stored using only the field list instead of th
The `field-id` property was added for each partition field in v2. In v1, the reference implementation assigned field ids sequentially in each spec starting at 1,000. See Partition Evolution for more details.
-In v3 metadata, writers must use only `source-ids` because v3 requires reader support for multi-arg transforms. In v1 and v2 metadata, writers must always write `source-id`; for multi-arg transforms, writers must produce `source-ids` and set `source-id` to the first ID from the field ID list.
+In v3 metadata, writers must use only `source-ids` because v3 requires reader support for multi-arg transforms.
Review Comment:
That's ok but I guess we have to clarify the expectations for the readers. Afair, for the readers, the spec says that if both `source-id` and `source-ids` are provided (in the case of an upgrade from V1/V2 to V3), then `source-id` should be used for single arg transforms and `source-ids` for multi arg transforms. I assume that if `source-id` and `source-ids` are provided in V3 (coming from V1/V2), single arg transforms should use the first element of `source-ids` and ignore `source-id`. I think we should clear the strategy here for the readers as well.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #12161:
URL: https://github.com/apache/iceberg/pull/12161#issuecomment-2641373672
Thanks @Fokko initially was hoping that we could support it for v1/v2 tables but understand the issues you are pointing out. I guess we can patiently wait v3 tables then!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #12161:
URL: https://github.com/apache/iceberg/pull/12161#issuecomment-2630172217
By the way I gonna start impl side of the thing :)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12161:
URL: https://github.com/apache/iceberg/pull/12161#discussion_r1938946299
##########
format/spec.md:
##########
@@ -1400,7 +1400,7 @@ In some cases partition specs are stored using only the field list instead of th
The `field-id` property was added for each partition field in v2. In v1, the reference implementation assigned field ids sequentially in each spec starting at 1,000. See Partition Evolution for more details.
-In v3 metadata, writers must use only `source-ids` because v3 requires reader support for multi-arg transforms. In v1 and v2 metadata, writers must always write `source-id`; for multi-arg transforms, writers must produce `source-ids` and set `source-id` to the first ID from the field ID list.
+In v3 metadata, writers must use only `source-ids` because v3 requires reader support for multi-arg transforms.
Review Comment:
You remember that correctly :) This PR doesn't touch the read-section:
https://github.com/apache/iceberg/blob/f6faa58dac57e03be6e02a43937ac7c15c770225/format/spec.md?plain=1#L1562-L1565
For V3 the `source-id` should be omitted:
https://github.com/apache/iceberg/blob/f6faa58dac57e03be6e02a43937ac7c15c770225/format/spec.md?plain=1#L1381
Let me know if you feel like more clarification is needed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #12161:
URL: https://github.com/apache/iceberg/pull/12161#issuecomment-2630237189
> By the way I gonna start impl side of the thing :)
SGTM, if you do the Java side, I'm happy to take care of PyIceberg 👍
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] spec: Remove `source-ids` for `V{1,2}` tables [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #12161:
URL: https://github.com/apache/iceberg/pull/12161#issuecomment-2636599919
@advancedxy Thanks for jumping in here. Yes, I hope to complete the V3 spec soon. By simplifying the impl as suggested in this spec, it will also be easier to implement in PyIceberg. Feel free to chime in on the dev list as well: https://lists.apache.org/thread/4njo7dmxmc50pmj9fw972bvo91r0bl8s This way it is also part of the official docs.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org