Core: Implement source-ids to deal with multi arguments transforms #12897

jbonofre · 2025-04-25T09:25:09Z

Following the update on the spec regarding source-id and source-ids (thanks again @Fokko 😄 ), here's the PR to introduce source-ids field in partition spec.

A few notes:

Internal representation is now based on source-ids
Serialization/deserialization supports source-id and source-ids elements in the json (exclusive), both populating source-ids internal representation (as List<Integer>)
The TestPartitionSpecParser is still testing source-id, but also source-ids parsing, and neither source-id and source-ids presence (throwing IllegalArgumentException in that case)
For backward compatibility (especially for the transforms), source id is still supported (using the first element in the internal representation) and some methods have been flagged as deprecated to encourage use of source ids.

jbonofre · 2025-04-25T09:26:17Z

@rdblue @Fokko @RussellSpitzer ^^ Thanks !

api/src/main/java/org/apache/iceberg/PartitionField.java

.palantir/revapi.yml

ajantha-bhat · 2025-04-25T12:41:08Z

api/src/main/java/org/apache/iceberg/PartitionSpec.java

-      fields.add(new PartitionField(sourceId, fieldId, name, transform));
+    Builder add(List<Integer> sourceIds, int fieldId, String name, Transform<?, ?> transform) {
+      // we use the first entry in the source-ids list here
+      checkAndAddPartitionName(name, sourceIds.get(0));


How does this work for multi arg partition field?
Do we need a logic that accepts the sourceIds and resolve the name of multi arg transform?

The sourceId here is to resolve conflict (no impact on name):

if (sourceColumnId != null) { // for identity transform case we allow conflicts between partition and schema field name // as // long as they are sourced from the same schema field Preconditions.checkArgument( schemaField == null || schemaField.fieldId() == sourceColumnId, "Cannot create identity partition sourced from different field in schema: %s", name);

Let me check if we should compare fieldId with each column id.

The current logic looks like

if (check conflicts). { if identity { make sure we aren't using a different field name for this identity transform } else { make sure we aren't matching any other column name } } Make sure it's not empty Make sure we haven't already used this name for another partition Add partition

I have no idea why those last 2 checks aren't part of the "if check conflicts" branch

Anyyyyyyway. I think this whole validation probably should be rewritten. The first branch we are checking basically based a lot of implicit assumptions when we should just be passing in the transform. But we don't have to do any of that now.

For now I think we should pass in sourceIds and just have the first branch include a "if sourceIds.length == 1"

OK, let me refactore this part.

core/src/main/java/org/apache/iceberg/PartitionSpecParser.java

api/src/main/java/org/apache/iceberg/PartitionField.java

RussellSpitzer · 2025-04-25T15:58:58Z

api/src/main/java/org/apache/iceberg/UnboundPartitionSpec.java


    for (UnboundPartitionField field : fields) {
-      Type fieldType = schema.findType(field.sourceId);
+      Type fieldType = schema.findType(field.sourceIds.get(0));


I know we don't have a multi-arg transform to resolve here yet but this doesn't seem like the right thing to do. I think Transforms.fromString needs to be modified to accept fromString(list[types], transformName)

Yes, agree. Let me update that.

api/src/main/java/org/apache/iceberg/UnboundPartitionSpec.java

core/src/main/java/org/apache/iceberg/PartitionSpecParser.java

RussellSpitzer · 2025-04-25T16:09:36Z

core/src/test/java/org/apache/iceberg/TestPartitionSpecParser.java

+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage(
+            "Cannot parse partition field, either source-id or source-ids has to be present");
+  }


We should have some tests for "toJson" as well?

We also need a check that the validation for identity transforms still holds true. Ie you cannot make a multi-arg transform with the same name as any of the columns

I added a test in TestPartitionSpecParser testing when neither source-id and source-ids are provided: testFromJsonWithoutSourceIdAndSourceIds().
I will add additional tests for toJson path too.

RussellSpitzer

Overall this looks pretty good to me. I think we are missing some testing but the approach looks correct. There are a few utility methods we need to fix up as well to accept sourceIds

jbonofre · 2025-04-25T16:17:17Z

Thanks @RussellSpitzer ! Let me fix the util methods and add tests. Thanks again !

RussellSpitzer · 2025-04-28T18:32:59Z

api/src/main/java/org/apache/iceberg/PartitionSpec.java

  static void checkCompatibility(PartitionSpec spec, Schema schema) {
    for (PartitionField field : spec.fields) {
-      Type sourceType = schema.findType(field.sourceId());
+      Type sourceType = schema.findType(field.sourceIds().get(0));


We should be extending this logic as well to handle multi arg sources

Yup. I'm doing a new update.

I introduced new canTransform(List) and getResultType(List) to support multi-args in transforms.
I didn't remove the "old" methods, and I propose to deprecate it.
Thoughts ?

jbonofre · 2025-05-16T09:00:44Z

Hey guys. I was traveling this week. I'm now back on this pr, updating according to the comments.

jbonofre · 2025-05-16T18:49:31Z

I did a first update to introduce multi-args in Transform. I will check/update the tests too.

api/src/main/java/org/apache/iceberg/PartitionSpec.java

api/src/main/java/org/apache/iceberg/UnboundPartitionSpec.java

jbonofre · 2025-05-23T19:33:03Z

Finally back from several trips, so resuming work on this PR.

jbonofre · 2025-06-02T15:23:50Z

I fixed the tests. I'm addressing the pending comments.

api/src/main/java/org/apache/iceberg/PartitionSpec.java

github-actions · 2025-08-02T00:18:45Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

github-actions · 2025-08-10T00:20:51Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

jbonofre · 2025-08-18T07:29:15Z

I'm resuming the work on this one (I have on vacation for the last two weeks).

github-actions · 2025-09-19T00:16:17Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

jbonofre · 2025-09-22T12:01:23Z

I just did a quick "rebase quick fix". I'm resuming my work on this one.

github-actions · 2025-10-23T00:17:17Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

github-actions · 2025-10-30T00:17:48Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

jbonofre · 2025-11-03T10:33:35Z

Here we go again 😄

jbonofre · 2025-12-01T14:35:49Z

Back again :)

jbonofre requested review from Fokko, RussellSpitzer and rdblue April 25, 2025 09:25

github-actions bot added API core labels Apr 25, 2025

jbonofre changed the title ~~Implement source-ids to deal with multi arguments transforms~~ Core: Implement source-ids to deal with multi arguments transforms Apr 25, 2025

ajantha-bhat reviewed Apr 25, 2025

View reviewed changes

jbonofre force-pushed the multi-arg-transforms branch 3 times, most recently from a5b96f2 to 549c197 Compare April 25, 2025 13:29