From 85270103bc84e02ec5c26bf9c6abbcec5cff5ac8 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Tue, 12 May 2020 17:58:08 -0700 Subject: [PATCH 1/2] Add assertions for sequence numbers to TestFastAppend. --- .../org/apache/iceberg/TableTestBase.java | 67 ++++++++++++++++++- .../org/apache/iceberg/TestFastAppend.java | 54 +++++++++++---- 2 files changed, 104 insertions(+), 17 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/TableTestBase.java b/core/src/test/java/org/apache/iceberg/TableTestBase.java index ef4466dec2c9..36d47681bc4a 100644 --- a/core/src/test/java/org/apache/iceberg/TableTestBase.java +++ b/core/src/test/java/org/apache/iceberg/TableTestBase.java @@ -29,6 +29,7 @@ import java.util.Iterator; import java.util.List; import java.util.Set; +import java.util.stream.LongStream; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.OutputFile; import org.apache.iceberg.types.Types; @@ -87,9 +88,13 @@ public class TableTestBase { public TestTables.TestTable table = null; protected final int formatVersion; + protected final Assertions V1Assert; + protected final Assertions V2Assert; public TableTestBase(int formatVersion) { this.formatVersion = formatVersion; + this.V1Assert = new Assertions(1, formatVersion); + this.V2Assert = new Assertions(2, formatVersion); } @Before @@ -197,6 +202,14 @@ ManifestEntry manifestEntry(ManifestEntry.Status status, Long snapshotId, DataFi } void validateSnapshot(Snapshot old, Snapshot snap, DataFile... newFiles) { + validateSnapshot(old, snap, null, newFiles); + } + + void validateSnapshot(Snapshot old, Snapshot snap, long sequenceNumber, DataFile... newFiles) { + validateSnapshot(old, snap, (Long) sequenceNumber, newFiles); + } + + void validateSnapshot(Snapshot old, Snapshot snap, Long sequenceNumber, DataFile... newFiles) { List oldManifests = old != null ? old.manifests() : ImmutableList.of(); // copy the manifests to a modifiable list and remove the existing manifests @@ -215,6 +228,10 @@ void validateSnapshot(Snapshot old, Snapshot snap, DataFile... newFiles) { for (ManifestEntry entry : ManifestFiles.read(manifest, FILE_IO).entries()) { DataFile file = entry.file(); + if (sequenceNumber != null) { + V1Assert.assertEquals("Sequence number should default to 0", 0, entry.sequenceNumber().longValue()); + V2Assert.assertEquals("Sequence number should match expected", sequenceNumber, entry.sequenceNumber()); + } Assert.assertEquals("Path should match expected", newPaths.next(), file.path().toString()); Assert.assertEquals("File's snapshot ID should match", id, (long) entry.snapshotId()); } @@ -242,12 +259,23 @@ List paths(DataFile... dataFiles) { return paths; } - static void validateManifest(ManifestFile manifest, - Iterator ids, - Iterator expectedFiles) { + void validateManifest(ManifestFile manifest, + Iterator ids, + Iterator expectedFiles) { + validateManifest(manifest, null, ids, expectedFiles); + } + + void validateManifest(ManifestFile manifest, + Iterator seqs, + Iterator ids, + Iterator expectedFiles) { for (ManifestEntry entry : ManifestFiles.read(manifest, FILE_IO).entries()) { DataFile file = entry.file(); DataFile expected = expectedFiles.next(); + if (seqs != null) { + V1Assert.assertEquals("Sequence number should default to 0", 0, entry.sequenceNumber().longValue()); + V2Assert.assertEquals("Sequence number should match expected", seqs.next(), entry.sequenceNumber()); + } Assert.assertEquals("Path should match expected", expected.path().toString(), file.path().toString()); Assert.assertEquals("Snapshot ID should match expected ID", @@ -280,6 +308,10 @@ static Iterator statuses(ManifestEntry.Status... statuses) return Iterators.forArray(statuses); } + static Iterator seqs(long... seqs) { + return LongStream.of(seqs).iterator(); + } + static Iterator ids(Long... ids) { return Iterators.forArray(ids); } @@ -291,4 +323,33 @@ static Iterator files(DataFile... files) { static Iterator files(ManifestFile manifest) { return ManifestFiles.read(manifest, FILE_IO).iterator(); } + + /** + * Used for assertions that only apply if the table version is v2. + */ + protected static class Assertions { + private final boolean enabled; + + private Assertions(int validForVersion, int formatVersion) { + this.enabled = validForVersion == formatVersion; + } + + void assertEquals(String context, int expected, int actual) { + if (enabled) { + Assert.assertEquals(context, expected, actual); + } + } + + void assertEquals(String context, long expected, long actual) { + if (enabled) { + Assert.assertEquals(context, expected, actual); + } + } + + void assertEquals(String context, Object expected, Object actual) { + if (enabled) { + Assert.assertEquals(context, expected, actual); + } + } + } } diff --git a/core/src/test/java/org/apache/iceberg/TestFastAppend.java b/core/src/test/java/org/apache/iceberg/TestFastAppend.java index 4f071e1570e7..4822d9218ac8 100644 --- a/core/src/test/java/org/apache/iceberg/TestFastAppend.java +++ b/core/src/test/java/org/apache/iceberg/TestFastAppend.java @@ -52,13 +52,21 @@ public void testEmptyTableAppend() { TableMetadata base = readMetadata(); Assert.assertNull("Should not have a current snapshot", base.currentSnapshot()); + Assert.assertEquals("Table should start with last-sequence-number 0", 0, base.lastSequenceNumber()); - Snapshot pending = table.newFastAppend() + table.newFastAppend() .appendFile(FILE_A) .appendFile(FILE_B) - .apply(); + .commit(); + + Snapshot snap = table.currentSnapshot(); + + validateSnapshot(base.currentSnapshot(), snap, 1, FILE_A, FILE_B); - validateSnapshot(base.currentSnapshot(), pending, FILE_A, FILE_B); + V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap.sequenceNumber()); + V2Assert.assertEquals("Last sequence number should be 1", 1, readMetadata().lastSequenceNumber()); + + V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber()); } @Test @@ -67,17 +75,25 @@ public void testEmptyTableAppendManifest() throws IOException { TableMetadata base = readMetadata(); Assert.assertNull("Should not have a current snapshot", base.currentSnapshot()); + Assert.assertEquals("Table should start with last-sequence-number 0", 0, base.lastSequenceNumber()); ManifestFile manifest = writeManifest(FILE_A, FILE_B); - Snapshot pending = table.newFastAppend() + table.newFastAppend() .appendManifest(manifest) - .apply(); + .commit(); - validateSnapshot(base.currentSnapshot(), pending, FILE_A, FILE_B); + Snapshot snap = table.currentSnapshot(); + + validateSnapshot(base.currentSnapshot(), snap, 1, FILE_A, FILE_B); // validate that the metadata summary is correct when using appendManifest Assert.assertEquals("Summary metadata should include 2 added files", - "2", pending.summary().get("added-data-files")); + "2", snap.summary().get("added-data-files")); + + V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap.sequenceNumber()); + V2Assert.assertEquals("Last sequence number should be 1", 1, readMetadata().lastSequenceNumber()); + + V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber()); } @Test @@ -86,22 +102,32 @@ public void testEmptyTableAppendFilesAndManifest() throws IOException { TableMetadata base = readMetadata(); Assert.assertNull("Should not have a current snapshot", base.currentSnapshot()); + Assert.assertEquals("Table should start with last-sequence-number 0", 0, base.lastSequenceNumber()); ManifestFile manifest = writeManifest(FILE_A, FILE_B); - Snapshot pending = table.newFastAppend() + table.newFastAppend() .appendFile(FILE_C) .appendFile(FILE_D) .appendManifest(manifest) - .apply(); + .commit(); + + Snapshot snap = table.currentSnapshot(); - long pendingId = pending.snapshotId(); + long commitId = snap.snapshotId(); - validateManifest(pending.manifests().get(0), - ids(pendingId, pendingId), + validateManifest(snap.manifests().get(0), + seqs(1, 1), + ids(commitId, commitId), files(FILE_C, FILE_D)); - validateManifest(pending.manifests().get(1), - ids(pendingId, pendingId), + validateManifest(snap.manifests().get(1), + seqs(1, 1), + ids(commitId, commitId), files(FILE_A, FILE_B)); + + V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap.sequenceNumber()); + V2Assert.assertEquals("Last sequence number should be 1", 1, readMetadata().lastSequenceNumber()); + + V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber()); } @Test From af51438ca9365255398ca9fe4c473d743a464435 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Tue, 12 May 2020 18:06:54 -0700 Subject: [PATCH 2/2] Fix checkstyle. --- core/src/test/java/org/apache/iceberg/TableTestBase.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/test/java/org/apache/iceberg/TableTestBase.java b/core/src/test/java/org/apache/iceberg/TableTestBase.java index 36d47681bc4a..e06af575bf15 100644 --- a/core/src/test/java/org/apache/iceberg/TableTestBase.java +++ b/core/src/test/java/org/apache/iceberg/TableTestBase.java @@ -88,7 +88,9 @@ public class TableTestBase { public TestTables.TestTable table = null; protected final int formatVersion; + @SuppressWarnings("checkstyle:MemberName") protected final Assertions V1Assert; + @SuppressWarnings("checkstyle:MemberName") protected final Assertions V2Assert; public TableTestBase(int formatVersion) {