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

[jbonofre]

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:

1. Internal representation is now based on source-ids
2. Serialization/deserialization supports source-id and source-ids elements in the json (exclusive), both populating source-ids internal representation (as List<Integer>)
3. 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)
4. 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]

@rdblue @Fokko @RussellSpitzer ^^ Thanks !

[ajantha-bhat]

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?

[jbonofre]

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.

[RussellSpitzer]

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"

[jbonofre]

OK, let me refactore this part.

[RussellSpitzer]

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)

[jbonofre]

Yes, agree. Let me update that.

[RussellSpitzer]

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

[jbonofre]

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]

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

[jbonofre]

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

[jbonofre]

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

[jbonofre]

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

[jbonofre]

Here we go again 😄

[jbonofre]

Back again :)

[jbonofre]

I'm back (again 😄 ) on Iceberg, resuming my different PRs. Resuming the work.

[RussellSpitzer]

This is broken now

[RussellSpitzer]

Since it indexes by the first sourceId i'm not sure what will actually happen, we probably need to do something here

[jbonofre]

I used a simple approach now, iterating on all source ids:

  for (PartitionField field : fields) {
      for (int sourceId : field.sourceIds()) {
          multiMap.put(sourceId, field);
      }
  }

I think it makes sense in this context.

Thoughts ?

[RussellSpitzer]

I think we should at least warn here if source id is also present and has a different value. I could also see throwing an exception but we generally don't fail unless we absolutely have to when parsing. As long as the writer would override the bad source-id I think we are ok

[jbonofre]

I'm hesitating here. To limit the impact, finally, I preferred to log (warn) when both source-ids and source-id are present (instead of failing).

Thoughts ?

[moomindani]

Hi @jbonofre, are you still planning to pursue this PR? I'm asking because I recently implemented the v3 read-tolerance side of this in PyIceberg (apache/iceberg-python#3630: fields with multiple source-ids parse and are treated as unknown transforms, with spec-compliant source-id/source-ids round-trip), and I'd like to help bring the Java implementation to that baseline — today, metadata containing a source-ids field cannot be read at all. Happy to help here if that's useful.