amogh-jahagirdar commented on code in PR #10760:
URL: https://github.com/apache/iceberg/pull/10760#discussion_r1697664698
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -217,6 +217,79 @@ public void close() throws IOException {
writer.close();
}
+ static class V3Writer extends ManifestWriter<DataFile> {
+ private final V3Metadata.IndexedManifestEntry<DataFile> entryWrapper;
+
+ V3Writer(PartitionSpec spec, EncryptedOutputFile file, Long snapshotId) {
Review Comment:
Oh nvm, forgot about inheritance of the snapshot ID
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -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);
Review Comment:
Nit: Position deletes *are* not supported .. .
##########
core/src/main/java/org/apache/iceberg/ManifestLists.java:
##########
@@ -66,6 +66,9 @@ static ManifestListWriter write(
case 2:
return new ManifestListWriter.V2Writer(
manifestListFile, snapshotId, parentSnapshotId, sequenceNumber);
+ case 3:
+ return new ManifestListWriter.V3Writer(
+ manifestListFile, snapshotId, parentSnapshotId, sequenceNumber);
Review 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)
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -217,6 +217,79 @@ public void close() throws IOException {
writer.close();
}
+ static class V3Writer extends ManifestWriter<DataFile> {
+ private final V3Metadata.IndexedManifestEntry<DataFile> entryWrapper;
+
+ V3Writer(PartitionSpec spec, EncryptedOutputFile file, Long snapshotId) {
Review Comment:
Shouldn't it be a `long` primitive snapshotID?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
