Core: Check for all specs in partitionsTable #7551
Conversation
|
cc: @szehon-ho |
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
Outdated
Show resolved
Hide resolved
|
@nastra: I have addressed the comments. Thanks for the review. @szehon-ho: Would you like to take a look at this? |
| @@ -805,6 +807,47 @@ public void testPartitionSpecEvolutionRemoval() { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void testPartitionSpecEvolutionToUnpartitioned() throws IOException { | |||
There was a problem hiding this comment.
Actually we have a test specifically for this: TestMetadataTableScansWithPartitionEvolution, for organization purpose? Could we move it there?
Also another question, does this test the case? I thought it was only failing on V2, because I had seen on V1 the partition transform remains as void on the latest spec, when removed.
There was a problem hiding this comment.
I moved it now.
Also another question, does this test the case? I thought it was only failing on V2, because I had seen on V1 the partition transform remains as void on the latest spec, when removed.
Yes, the testcase fails only for v2 without the PR changes as V1 will have void transform to satisfy the check. But I thought it can run for both.
BTW, now I removed the scan code in the testcase because my intention was to just scan spec_id column and see it is filled instead of null. But I don't know how to achieve it (In sql or spark code I know. But in this table scan I am not sure as instead of rows it returns content files)
There was a problem hiding this comment.
Thanks.
I think the original asserts you had before moving the test , worked pretty well? I would think it's nice to have those. But up to you.
| @@ -222,6 +222,14 @@ public void testPositionDeletesPartitionSpecRemoval() { | |||
| constantsMap(posDeleteTask, partitionType).get(MetadataColumns.FILE_PATH.fieldId())); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testPartitionSpecEvolutionToUnpartitioned() { | |||
There was a problem hiding this comment.
feels like test name is not super clear, maybe testPreviouslyPartitionedSpecStillShowPartitionColumnWhenEvolveToUnpartitioned ?
There was a problem hiding this comment.
Hm. it looks a bit long :) In that case, Id vote to maybe bring back the original asserts then the first commit so its a more end-to-end test and we dont have to change the name too much , 54b95ea as I dont think its covered elsewhere (though realize its not related to this pr )
|
Merged, thanks @ajantha-bhat and @nastra for additional review! |
Partitions Table schema checks only current spec to decide if partition column is returned or not. We should actually check historical specs to see if it was ever partitioned, similar to Files/Entries and other metadata tables.
Fixes #7533