Core: Handle partition evolution case in PartitionStatsUtil#computeStats

[deniskuzZ]

Description:
PartitionMap creates internal wrappers based on table specs partitionType, however, we are passing coercedPartition with unified partition type considering all specs as a KEY, leading to wrong evaluation.

#12168

Repro:
https://github.com/apache/hive/blob/master/iceberg/iceberg-handler/src/test/queries/positive/iceberg_bucket_map_join_4.q

Testing:

• TestPartitionStatsUtil#testPartitionStatsWithSchemaEvolution2

[deniskuzZ]

@ajantha-bhat, @aokolnychyi, could you please take a look?

[deniskuzZ]

ping @ajantha-bhat

[ajantha-bhat]

Should use junit5 like other tests. (Used wrong annotation?)

I don't think the tests are running right now.
So, please fix it and I will try to understand the test case.

The original intention is to store the unified tuple as the key itself. So, it can handle all the partition evolution. So, from the test I am not yet clear what was expected and what went wrong easily.

[deniskuzZ]

thanks @ajantha-bhat for checking the PR, I almost gave up expecting an answer :)

1. removed the wrong test annotation
2. I understood the intention, however, there is a bug with the key matching: PartitionMap is initialized with individual partition specs [], but the supplied key is a unified tuple.

For the created test case, the existing code returns just 3 partition values instead of 6. The test simulates partition evolution by changing the number of buckets from 2 to 4: spec(c2, bucket(c1, 2)) -> spec(c2, bucket(c1, 4))

[foo, 0, null]
[foo, 1, null]
[bar, null, 0]

[deniskuzZ]

Expecting actual:
  [(PartitionData{c2=foo, c1_bucket=0, c1_bucket_4=null}, 0, 100290L, 1, 38596L, 0L, 0, 0L, 0, 0L, 1739473244037L, 1L),
    (PartitionData{c2=foo, c1_bucket=1, c1_bucket_4=null}, 0, 100293L, 1, 43840L, 0L, 0, 0L, 0, 0L, 1739473244037L, 1L),
    (PartitionData{c2=bar, c1_bucket=null, c1_bucket_4=0}, 1, 402736L, 4, 46675L, 0L, 0, 0L, 0, 0L, 1739473244341L, 2L)]
to contain exactly in any order:
  [(PartitionData{c2=foo, c1_bucket=0, c1_bucket_4=null}, 0, 100290L, 1, 38596L, 0L, 0, 0L, 0, 0L, 1739473244037L, 1L),
    (PartitionData{c2=foo, c1_bucket=1, c1_bucket_4=null}, 0, 100293L, 1, 43840L, 0L, 0, 0L, 0, 0L, 1739473244037L, 1L),
    (PartitionData{c2=bar, c1_bucket=null, c1_bucket_4=0}, 1, 100747L, 1, 5616L, 0L, 0, 0L, 0, 0L, 1739473244341L, 2L),
    (PartitionData{c2=bar, c1_bucket=null, c1_bucket_4=1}, 1, 100122L, 1, 7260L, 0L, 0, 0L, 0, 0L, 1739473244341L, 2L),
    (PartitionData{c2=bar, c1_bucket=null, c1_bucket_4=2}, 1, 100914L, 1, 13793L, 0L, 0, 0L, 0, 0L, 1739473244341L, 2L),
    (PartitionData{c2=bar, c1_bucket=null, c1_bucket_4=3}, 1, 100953L, 1, 20006L, 0L, 0, 0L, 0, 0L, 1739473244341L, 2L)]

[ajantha-bhat]

I see,

PartitionMap is initialized with individual partition specs [], but the supplied key is a unified tuple

I assumed partitionMap will use the key supplied. Now I see that it is based on the individual spec

iceberg/core/src/main/java/org/apache/iceberg/util/PartitionMap.java

Line 181 in bcbbd03

return StructLikeMap.create(spec.partitionType());

Since, we don't combine stats for different spec id, your solution works.

Thanks for the fix.

[ajantha-bhat]

LGTM.

@aokolnychyi: Could you please help in merging this PR?

[ajantha-bhat]

PR title can be Core: Handle partition evolution case in partition stats compute

[pvary]

I'm a bit concerned about these casts.

Could keyTemplate.copyFor(file.partition()) work?

[deniskuzZ]

hi @pvary, thanks for taking a look.
keyTemplate is a unified tuple struct, so it can't be used. btw, if you grep iceberg code, you'll find a number of same casts.

[pvary]

Seems like the tests are broken by the infra 😢

Could you help me understand the original issue? I thought that I understand, but seems like I still miss something.

I get that there is difference between the partitionType (which is the unified spec), the coercedPartition which is the partition key for the unified partition type.

What is the key we would like to see in the statsMap?

[deniskuzZ]

discussed in private. @pvary, please let me know if any questions left.

[pvary]

Discussed this with @RussellSpitzer, and he confirmed that casting is indeed correct.

[pvary]

+1 LGTM
When the infra recovers, and ww have a green run, we can commit

[pvary]

Merged to master.
Thanks @deniskuzZ for the fix and @ajantha-bhat for the review!