Core: Metadata table queries fail if a partition column was reused in V2

[szlta]

First go at fixing #4661.
The proposed solution described at #3411 (comment) seems very hardly feasible, so this would be a quick fix to just avoid Invalid schema errors thrown in any metadata query where the a partition col was reused.

[szehon-ho]

I still need to ramp a bit on this, but I thought in that discussion we are going to collapse all of the same partition fields (if some are re-used) and they are uniquely identified with specid? Ie, just "data" and "id".

example for reference:

PartitionSpec initialSpec = PartitionSpec.builderFor(SCHEMA)
    .identity("data")
    .build();
TestTables.TestTable table = TestTables.create(tableDir, "test", SCHEMA, initialSpec, V2_FORMAT_VERSION);

table.updateSpec()
    .removeField("data")
    .commit();

table.updateSpec()
    .addField("data")
    .addField("id")
    .commit();

struct<1000: data: optional string, 1001: data: optional string, 1002: id: optional int>

I am wondering, does the code try to re-use old partition values in both metadata (ManifestEvaluator) and data filtering? I thought it does, but need to check, let me know if assumption is incorrect. Example here, we would hopefully use "data" partition values written by the table's initial spec, for queries after re-adding "data" to the spec. So then it may make sense to collapse the fields together in the partitions/metadata table? Welcome any thoughts from @rdblue , @aokolnychyi who have more background in the initial discussion.

[szlta]

Hey @szehon-ho this was just intended to be a quick fix so we don't throw exceptions for the case you mentioned too.
Currently if we want to collapse the matching partition columns we will hit the following issue:

• suppose we'd have 1000: data, 1001: data, 1002: id in the combined partition type
• Partitioning.partitionType could collapse this into 1001: data, 1002: id (leaving out the last seen "data" as going back in specs)
• this will be the "read schema" when reading the manifest files (as the underlying avro files)
• for spec0, id matching will think that although the file schema has 1000: data, but the read schema doesn't, this reading will just return null for 1001: data and null for 1002: id leaving us no info the collapse upon..

Perhaps if we could rework the re-addition of "data" column to spec so that no new field ID is generated for it? I'm not sure if this will cause other issues though..

[szehon-ho]

Yea I understand, my gist of the earlier conversation was that the most optimal solution is to try to re-use the field-id if its the equivalent transformation as a partition field before? I think the thing to check would be whether the regular metadata filtering is affected or not (today it seems to work, via logic in Projection filter being able to resolve it)

[szlta]

@szehon-ho I think we were probably talking about the same thing for generating no new ID for a PartitionField that was already seen. I have amended my change based on this. I looked into this question in some detail and have found that this is actually kind of already happening when we are removing part of the PartitionFields:

• we start with
  spec0: 1000: identity(a), 1001: identity(b)
• then if update the spec so that only b remains, a doesn't get reassigned a new fieldID, although we do create a new spec:
  spec1: 1000: identity(a)
• now if we re-add b and c, then b would get assigned ID 1002 and c would get ID 1003
• I propose we look through in the past specs to see if b as partition field already participated (based on transform function and source IDs) and recycle the old ID, so we end up with:
  spec2: 1000: identity(a), 1001: identity(b), 1002: identity(c)

Looking at the tests this should work, there's only one failure I get now in TestMetadataTableScans.testFilesTableScanWithDroppedPartition:

Expected :struct<1000: data_bucket: optional int, 1001: data_bucket_16: optional int, 1002: data_trunc_2: optional string>
Actual   :struct<1000: data_bucket_16: optional int, 1001: data_trunc_2: optional string>

This made me think though.. it seems like we have two different ways of generating the partition field column name:
https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java#L460 generates data_bucket_16, while
https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L484 generates data_bucket. Does anyone know why this discrepancy exists? My gut feeling is that the former would be the correct way.

[szehon-ho]

This change looks like it's the right direction for me.

Is there any way to validate the data scan is correct filtering out the right files in this scenario (when we remove and add back partition filter), maybe by adding a test in "TestDataTableScan"?

In regards to your question in TestMetadataTableScans.testFilesTableScanWithDroppedPartition, yes I agree the former seems the correct name to me and seems we should try to align it, but I don't really have much context and maybe @aokolnychyi , @rdblue or @RussellSpitzer have more thoughts about that.

[szehon-ho]

This looks better to me, just some minor comments and some test failures.

[szlta]

@aokolnychyi , @rdblue or @RussellSpitzer could you please share your opinion on #4662 (review) aka on the discrepancy in generating PartitionField names?

[pvary]

Since the decision on #4868 is to stick with the old naming convention of default partition names, I think this is the best solution here. Please call out if you disagree, otherwise I will merge this soon.

Thanks,
Peter

[pvary]

Thanks for the fix @szlta and the @szehon-ho and @rdblue for the review!