Core: Adds Basic Classes for Iceberg Table Version 3 #10760
Conversation
RussellSpitzer
requested review from
rdblue,
nastra,
aokolnychyi and
amogh-jahagirdar
| @@ -0,0 +1,575 @@ | |||
| /* | |||
There was a problem hiding this comment.
This is a direct copy of V2Metadata.json
There was a problem hiding this comment.
Thanks @RussellSpitzer. I looked at the changes. They look good to me. Is the plan to add V3 specific changes in later PRs?
Yes! If you check on the right, there is a link to the Project where i'm trying to collect other required changes. I wanted to make sure we had all the base classes setup so folks can start working on the specific new implementations. |
|
Sorry for the delay on reviewing @RussellSpitzer I'm taking a look! |
| @@ -1574,7 +1573,7 @@ public void testPositionDeletesWithBaseTableFilterNot() { | |||
|
|
|||
| @TestTemplate | |||
| public void testPositionDeletesResiduals() { | |||
| assumeThat(formatVersion).as("Position deletes supported only for v2 tables").isEqualTo(2); | |||
| assumeThat(formatVersion).as("Position deletes not supported by V1 Tables").isNotEqualTo(1); | |||
There was a problem hiding this comment.
Nit: Position deletes are not supported .. .
| static class V3Writer extends ManifestWriter<DataFile> { | ||
| private final V3Metadata.IndexedManifestEntry<DataFile> entryWrapper; | ||
|
|
||
| V3Writer(PartitionSpec spec, EncryptedOutputFile file, Long snapshotId) { |
There was a problem hiding this comment.
Shouldn't it be a long primitive snapshotID?
There was a problem hiding this comment.
Oh nvm, forgot about inheritance of the snapshot ID
| case 3: | ||
| return new ManifestListWriter.V3Writer( | ||
| manifestListFile, snapshotId, parentSnapshotId, sequenceNumber); |
There was a problem hiding this comment.
Something I've been thinking about is abstracting away the different fields required for V1/V2/V3 without having to have a separate writer implementation for each. But it may not be worth it since the number of table format versions doesn't change too much and that sort of refactoring can happen down the line if it's determined to be needed (since these classes are package private, not in the public API I think there's that flexibility)
There was a problem hiding this comment.
Yeah I was figuring for now we keep things the same as between V1 and V2, also protects us a little more from inadvertently breaking V2 as we begin mucking about with V3.
|
Thanks @nastra and @amogh-jahagirdar ! Let's get to work on V3 now :) |
Fixes #10747