You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/06/09 16:38:52 UTC
[GitHub] [iceberg] chenjunjiedada opened a new pull request #1101: Add sequence number assertions in merge append unit tests
chenjunjiedada opened a new pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r466024280
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A);
ManifestFile manifest2 = writeManifestWithName("FILE_C", FILE_C);
ManifestFile manifest3 = writeManifestWithName("FILE_D", FILE_D);
+ ManifestFile manifest4 = writeManifestWithName("FILE_B", FILE_B);
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
+ Snapshot snap1 = table.currentSnapshot();
+ long commitId1 = snap1.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 2 merged manifest for first write",
2, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap1.allManifests().get(0),
+ seqs(1),
+ ids(commitId1),
+ files(FILE_A));
+ validateManifest(snap1.allManifests().get(1),
+ seqs(1, 1),
+ ids(commitId1, commitId1),
+ files(FILE_C, FILE_D));
+
+ V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap1.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
-
+ Snapshot snap2 = table.currentSnapshot();
+ long commitId2 = snap2.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 3 merged manifest for second write",
3, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap2.allManifests().get(0),
+ seqs(2),
+ ids(commitId2),
+ files(FILE_A));
+
+ validateManifest(snap2.allManifests().get(1),
+ seqs(2, 2),
+ ids(commitId2, commitId2),
+ files(FILE_C, FILE_D));
+
+ validateManifestEntries(snap2.allManifests().get(2),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D),
+ statuses(Status.EXISTING, Status.EXISTING, Status.EXISTING));
+
+ validateManifest(snap2.allManifests().get(2),
+ seqs(1, 1, 1),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D));
+ V2Assert.assertEquals("Snapshot sequence number should be 2", 2, snap2.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 2", 2, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
// validate that the metadata summary is correct when using appendManifest
Assert.assertEquals("Summary metadata should include 3 added files",
"3", readMetadata().currentSnapshot().summary().get("added-data-files"));
+
+ table.updateProperties()
Review comment:
If there are more cases that were not originally tested, then let's create new test methods for them with names that describe what is happening.
What are the additional cases?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r438453541
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
Review comment:
OK.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#issuecomment-670803900
Sorry, I thought I'd clicked merge earlier today. It's done now, though. Thanks @chenjunjiedada!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r438454994
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -251,7 +317,10 @@ public void testMergeWithExistingManifestAfterDelete() {
long baseId = base.currentSnapshot().snapshotId();
Assert.assertEquals("Should create 1 manifest for initial write",
1, base.currentSnapshot().allManifests().size());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, readMetadata().lastSequenceNumber());
Review comment:
OK.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r466023724
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -75,17 +85,30 @@ public void testEmptyTableAppendManifest() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ Assert.assertEquals("Last sequence number should be 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A, FILE_B);
- Snapshot pending = table.newAppend()
+ table.newAppend()
.appendManifest(manifest)
- .apply();
+ .commit();
+
+ Snapshot committedSnapshot = table.currentSnapshot();
+ Assert.assertNotNull("Should create a snapshot", table.currentSnapshot());
+ V1Assert.assertEquals("Last sequence number should be 0", 0, table.ops().current().lastSequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, table.ops().current().lastSequenceNumber());
- validateSnapshot(base.currentSnapshot(), pending, FILE_A, FILE_B);
+ validateSnapshot(base.currentSnapshot(), committedSnapshot, FILE_A, FILE_B);
+
+ long snapshotId = committedSnapshot.snapshotId();
+ validateManifest(committedSnapshot.allManifests().get(0),
Review comment:
Before accessing a list item using `get`, you should always assert that the list has the number of items you expect. That helps avoid problems where you're validating the wrong item because there are more than you expected, and also provides a better error when there are no items at all.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#issuecomment-642317341
@rdblue , I didn't add assertions of the test cases have a base commit and then call `apply` function or inject the failure commit. I will add them in next commit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r438454831
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A);
ManifestFile manifest2 = writeManifestWithName("FILE_C", FILE_C);
ManifestFile manifest3 = writeManifestWithName("FILE_D", FILE_D);
+ ManifestFile manifest4 = writeManifestWithName("FILE_B", FILE_B);
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
+ Snapshot snap1 = table.currentSnapshot();
+ long commitId1 = snap1.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 2 merged manifest for first write",
2, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap1.allManifests().get(0),
+ seqs(1),
+ ids(commitId1),
+ files(FILE_A));
+ validateManifest(snap1.allManifests().get(1),
+ seqs(1, 1),
+ ids(commitId1, commitId1),
+ files(FILE_C, FILE_D));
+
+ V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap1.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
-
+ Snapshot snap2 = table.currentSnapshot();
+ long commitId2 = snap2.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 3 merged manifest for second write",
3, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap2.allManifests().get(0),
+ seqs(2),
+ ids(commitId2),
+ files(FILE_A));
+
+ validateManifest(snap2.allManifests().get(1),
+ seqs(2, 2),
+ ids(commitId2, commitId2),
+ files(FILE_C, FILE_D));
+
+ validateManifestEntries(snap2.allManifests().get(2),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D),
+ statuses(Status.EXISTING, Status.EXISTING, Status.EXISTING));
+
+ validateManifest(snap2.allManifests().get(2),
+ seqs(1, 1, 1),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D));
+ V2Assert.assertEquals("Snapshot sequence number should be 2", 2, snap2.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 2", 2, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
// validate that the metadata summary is correct when using appendManifest
Assert.assertEquals("Summary metadata should include 3 added files",
"3", readMetadata().currentSnapshot().summary().get("added-data-files"));
+
+ table.updateProperties()
Review comment:
Both of them are related to merging manifests according to minMergeCount. Does that make sense?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r438375936
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A);
ManifestFile manifest2 = writeManifestWithName("FILE_C", FILE_C);
ManifestFile manifest3 = writeManifestWithName("FILE_D", FILE_D);
+ ManifestFile manifest4 = writeManifestWithName("FILE_B", FILE_B);
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
+ Snapshot snap1 = table.currentSnapshot();
+ long commitId1 = snap1.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 2 merged manifest for first write",
2, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap1.allManifests().get(0),
Review comment:
New `validateManifest` calls should also supply the [status of each file](https://github.com/apache/iceberg/blob/master/core/src/test/java/org/apache/iceberg/TableTestBase.java#L302-L306) as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#issuecomment-664025221
@chenjunjiedada, the tests originally used `apply` to test the logic in the merge append implementation. It wasn't necessary to actually commit because the logic to add a snapshot was tested separately. But now, adding sequence number assertions should actually perform commits and check the sequence numbers after commit. Otherwise, this doesn't really test sequence numbers for all of the cases because some cases never commit.
For example, `testEmptyTableAppend` should be updated like this:
```java
@Test
public void testEmptyTableAppend() {
Assert.assertEquals("Table should start empty", 0, listManifestFiles().size());
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
Assert.assertEquals("Last sequence number should start at 0", 0, base.lastSequenceNumber());
table.newAppend()
.appendFile(FILE_A)
.appendFile(FILE_B)
.commit(); // <-- changed to commit
Snapshot committed = table.currentSnapshot();
Assert.assertNotNull("Should create a snapshot", table.currentSnapshot());
V1Assert.assertEquals("Last sequence number should be 0", 0, table.ops().current().lastSequenceNumber());
V2Assert.assertEquals("Last sequence number should be 1", 1, table.ops().current().lastSequenceNumber());
Assert.assertEquals("Should create 1 manifest for initial write",
1, committed.allManifests().size());
long pendingId = committed.snapshotId();
validateManifest(committed.allManifests().get(0),
seqs(1, 1),
ids(pendingId, pendingId),
files(FILE_A, FILE_B),
statuses(Status.ADDED, Status.ADDED));
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r467144625
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -193,46 +259,200 @@ public void testManifestMergeMinCount() throws IOException {
.appendManifest(manifest3)
.commit();
+ Snapshot snap1 = table.currentSnapshot();
+ long commitId1 = snap1.snapshotId();
+ base = readMetadata();
+ V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap1.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
+
Assert.assertEquals("Should contain 2 merged manifest for first write",
2, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap1.allManifests().get(0),
+ seqs(1),
+ ids(commitId1),
+ files(FILE_A),
+ statuses(Status.ADDED));
+ validateManifest(snap1.allManifests().get(1),
+ seqs(1, 1),
+ ids(commitId1, commitId1),
+ files(FILE_C, FILE_D),
+ statuses(Status.ADDED, Status.ADDED));
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
+ Snapshot snap2 = table.currentSnapshot();
+ long commitId2 = snap2.snapshotId();
+ base = readMetadata();
+ V2Assert.assertEquals("Snapshot sequence number should be 2", 2, snap2.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 2", 2, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
Assert.assertEquals("Should contain 3 merged manifest for second write",
3, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap2.allManifests().get(0),
+ seqs(2),
+ ids(commitId2),
+ files(FILE_A),
+ statuses(Status.ADDED));
+ validateManifest(snap2.allManifests().get(1),
+ seqs(2, 2),
+ ids(commitId2, commitId2),
+ files(FILE_C, FILE_D),
+ statuses(Status.ADDED, Status.ADDED));
+ validateManifest(snap2.allManifests().get(2),
+ seqs(1, 1, 1),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D),
+ statuses(Status.EXISTING, Status.EXISTING, Status.EXISTING));
// validate that the metadata summary is correct when using appendManifest
Assert.assertEquals("Summary metadata should include 3 added files",
"3", readMetadata().currentSnapshot().summary().get("added-data-files"));
}
+ @Test
+ public void testManifestsMergeIntoOne() throws IOException {
+ Assert.assertEquals("Table should start empty", 0, listManifestFiles().size());
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot snap1 = table.currentSnapshot();
+ TableMetadata base = readMetadata();
+ V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap1.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
+ long commitId1 = snap1.snapshotId();
+
+ Assert.assertEquals("Should contain 1 manifest", 1, snap1.allManifests().size());
+ validateManifest(snap1.allManifests().get(0), seqs(1), ids(commitId1), files(FILE_A), statuses(Status.ADDED));
+
+ table.newAppend().appendFile(FILE_B).commit();
+ Snapshot snap2 = table.currentSnapshot();
+ long commitId2 = snap2.snapshotId();
+ base = readMetadata();
+ V2Assert.assertEquals("Snapshot sequence number should be 2", 2, snap2.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 2", 2, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
+
+ Assert.assertEquals("Should contain 2 manifests", 2, snap2.allManifests().size());
+ validateManifest(snap2.allManifests().get(0),
+ seqs(2),
+ ids(commitId2),
+ files(FILE_B),
+ statuses(Status.ADDED));
+ validateManifest(snap2.allManifests().get(1),
+ seqs(1),
+ ids(commitId1),
+ files(FILE_A),
+ statuses(Status.ADDED));
+
+ table.newAppend()
+ .appendManifest(writeManifest("input-m0.avro",
+ manifestEntry(ManifestEntry.Status.ADDED, null, FILE_C)))
+ .commit();
+ Snapshot snap3 = table.currentSnapshot();
+
+ base = readMetadata();
+ V2Assert.assertEquals("Snapshot sequence number should be 3", 3, snap3.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 3", 3, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
+
+ Assert.assertEquals("Should contain 3 manifests", 3, snap3.allManifests().size());
+ long commitId3 = snap3.snapshotId();
+ validateManifest(snap3.allManifests().get(0),
+ seqs(3),
+ ids(commitId3),
+ files(FILE_C),
+ statuses(Status.ADDED));
+ validateManifest(snap3.allManifests().get(1),
+ seqs(2),
+ ids(commitId2),
+ files(FILE_B),
+ statuses(Status.ADDED));
+ validateManifest(snap3.allManifests().get(2),
+ seqs(1),
+ ids(commitId1),
+ files(FILE_A),
+ statuses(Status.ADDED));
+
+ table.updateProperties()
+ .set(TableProperties.MANIFEST_MIN_MERGE_COUNT, "1")
+ .commit();
+
+ table.newAppend()
+ .appendManifest(writeManifest("input-m1.avro",
+ manifestEntry(ManifestEntry.Status.ADDED, null, FILE_D)))
+ .commit();
+ Snapshot snap4 = table.currentSnapshot();
+
+ base = readMetadata();
+ V2Assert.assertEquals("Snapshot sequence number should be 4", 4, snap4.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 4", 4, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
+
+ long commitId4 = snap4.snapshotId();
+ Assert.assertEquals("Should only contains 1 merged manifest", 1, snap4.allManifests().size());
+ validateManifest(snap4.allManifests().get(0),
+ seqs(4, 3, 2, 1),
+ ids(commitId4, commitId3, commitId2, commitId1),
+ files(FILE_D, FILE_C, FILE_B, FILE_A),
+ statuses(Status.ADDED, Status.EXISTING, Status.EXISTING, Status.EXISTING));
+ }
+
@Test
public void testManifestDoNotMergeMinCount() throws IOException {
Assert.assertEquals("Table should start empty", 0, listManifestFiles().size());
table.updateProperties().set("commit.manifest.min-count-to-merge", "4").commit();
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V1Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
Review comment:
Minor: we don't need duplicate assertions when the condition should be true for both v1 and v2 tables.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r438372887
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
Review comment:
This doesn't make sense to me. The table metadata was just read into `base` above, so the `V2Assert` line doesn't need to read it again and can use `base.lastSequenceNumber()`. Next, if both v1 and v2 assertions are that `base.lastSequenceNumber()` is 0, then I think it makes more sense to use a normal `Assert.assertEquals` because it doesn't depend on the table version.
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
Review comment:
This doesn't make sense to me. The table metadata was just read into `base` above, so the `V2Assert` line doesn't need to read it again and can use `base.lastSequenceNumber()`. Next, if both v1 and v2 assertions are that `base.lastSequenceNumber()` is 0, then I think it makes more sense to use a normal `Assert.assertEquals` because it doesn't depend on the table version.
I think these two lines should be replaced by this:
```java
Assert.assertEquals("Should have last sequence number 0", 0, base.lastSequenceNumber());
```
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
Review comment:
This doesn't make sense to me. The table metadata was just read into `base` above, so the `V2Assert` line doesn't need to read it again and can use `base.lastSequenceNumber()`. Next, if both v1 and v2 assertions are that `base.lastSequenceNumber()` is 0, then I think it makes more sense to use a normal `Assert.assertEquals` because it doesn't depend on the table version.
I think these two lines should be replaced by this:
```java
Assert.assertEquals("Last sequence number should be 0", 0, base.lastSequenceNumber());
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r467142266
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -121,56 +156,86 @@ public void testMergeWithAppendFilesAndManifest() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ Assert.assertEquals("Last sequence number should be 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A, FILE_B);
- Snapshot pending = table.newAppend()
+ table.newAppend()
.appendFile(FILE_C)
.appendFile(FILE_D)
.appendManifest(manifest)
- .apply();
+ .commit();
+
+ Snapshot committedSnapshot = table.currentSnapshot();
+ Assert.assertNotNull("Should create a snapshot", table.currentSnapshot());
+ V1Assert.assertEquals("Last sequence number should be 0", 0, table.ops().current().lastSequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, table.ops().current().lastSequenceNumber());
- long pendingId = pending.snapshotId();
+ long snapshotId = committedSnapshot.snapshotId();
- Assert.assertEquals("Should create 1 merged manifest", 1, pending.allManifests().size());
- validateManifest(pending.allManifests().get(0),
- ids(pendingId, pendingId, pendingId, pendingId),
- files(FILE_C, FILE_D, FILE_A, FILE_B));
+ Assert.assertEquals("Should create 1 merged manifest", 1, committedSnapshot.allManifests().size());
+
+ validateManifest(committedSnapshot.allManifests().get(0),
+ seqs(1, 1, 1, 1),
+ ids(snapshotId, snapshotId, snapshotId, snapshotId),
+ files(FILE_C, FILE_D, FILE_A, FILE_B),
+ statuses(Status.ADDED, Status.ADDED, Status.ADDED, Status.ADDED)
+ );
}
@Test
public void testMergeWithExistingManifest() {
// merge all manifests for this test
table.updateProperties().set("commit.manifest.min-count-to-merge", "1").commit();
-
+ V1Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
Assert.assertEquals("Table should start empty", 0, listManifestFiles().size());
table.newAppend()
.appendFile(FILE_A)
.appendFile(FILE_B)
.commit();
+ Assert.assertNotNull("Should create a snapshot", table.currentSnapshot());
+ V1Assert.assertEquals("Last sequence number should be 0", 0, table.ops().current().lastSequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, table.ops().current().lastSequenceNumber());
+
TableMetadata base = readMetadata();
- long baseId = base.currentSnapshot().snapshotId();
+ Snapshot commitBefore = table.currentSnapshot();
+ long baseId = commitBefore.snapshotId();
+ validateSnapshot(null, commitBefore, 1, FILE_A, FILE_B);
+
Assert.assertEquals("Should create 1 manifest for initial write",
- 1, base.currentSnapshot().allManifests().size());
+ 1, commitBefore.allManifests().size());
ManifestFile initialManifest = base.currentSnapshot().allManifests().get(0);
+ validateManifest(initialManifest,
+ seqs(1, 1),
+ ids(baseId, baseId),
+ files(FILE_A, FILE_B),
+ statuses(Status.ADDED, Status.ADDED));
- Snapshot pending = table.newAppend()
+ table.newAppend()
.appendFile(FILE_C)
.appendFile(FILE_D)
- .apply();
+ .commit();
+ V1Assert.assertEquals("Last sequence number should be 0", 0, table.ops().current().lastSequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 2, table.ops().current().lastSequenceNumber());
Review comment:
Nit: context is incorrect.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r466023140
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -75,17 +85,30 @@ public void testEmptyTableAppendManifest() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ Assert.assertEquals("Last sequence number should be 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A, FILE_B);
- Snapshot pending = table.newAppend()
+ table.newAppend()
.appendManifest(manifest)
- .apply();
+ .commit();
+
+ Snapshot committedSnapshot = table.currentSnapshot();
+ Assert.assertNotNull("Should create a snapshot", table.currentSnapshot());
+ V1Assert.assertEquals("Last sequence number should be 0", 0, table.ops().current().lastSequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, table.ops().current().lastSequenceNumber());
- validateSnapshot(base.currentSnapshot(), pending, FILE_A, FILE_B);
+ validateSnapshot(base.currentSnapshot(), committedSnapshot, FILE_A, FILE_B);
Review comment:
Is this validation still needed if `validateManifest` is called below?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#issuecomment-642229244
Thanks for working on this, @chenjunjiedada. I noticed that there are quite a few tests in this suite that you didn't add sequence number assertions to. How did you choose which ones to add assertions to and which ones to skip?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r466414959
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -75,17 +85,30 @@ public void testEmptyTableAppendManifest() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ Assert.assertEquals("Last sequence number should be 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A, FILE_B);
- Snapshot pending = table.newAppend()
+ table.newAppend()
.appendManifest(manifest)
- .apply();
+ .commit();
+
+ Snapshot committedSnapshot = table.currentSnapshot();
+ Assert.assertNotNull("Should create a snapshot", table.currentSnapshot());
+ V1Assert.assertEquals("Last sequence number should be 0", 0, table.ops().current().lastSequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, table.ops().current().lastSequenceNumber());
- validateSnapshot(base.currentSnapshot(), pending, FILE_A, FILE_B);
+ validateSnapshot(base.currentSnapshot(), committedSnapshot, FILE_A, FILE_B);
+
+ long snapshotId = committedSnapshot.snapshotId();
+ validateManifest(committedSnapshot.allManifests().get(0),
Review comment:
I got it. I added the assertion in this one as well as missing assertions in other test methods.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r466413196
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A);
ManifestFile manifest2 = writeManifestWithName("FILE_C", FILE_C);
ManifestFile manifest3 = writeManifestWithName("FILE_D", FILE_D);
+ ManifestFile manifest4 = writeManifestWithName("FILE_B", FILE_B);
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
+ Snapshot snap1 = table.currentSnapshot();
+ long commitId1 = snap1.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 2 merged manifest for first write",
2, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap1.allManifests().get(0),
+ seqs(1),
+ ids(commitId1),
+ files(FILE_A));
+ validateManifest(snap1.allManifests().get(1),
+ seqs(1, 1),
+ ids(commitId1, commitId1),
+ files(FILE_C, FILE_D));
+
+ V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap1.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
-
+ Snapshot snap2 = table.currentSnapshot();
+ long commitId2 = snap2.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 3 merged manifest for second write",
3, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap2.allManifests().get(0),
+ seqs(2),
+ ids(commitId2),
+ files(FILE_A));
+
+ validateManifest(snap2.allManifests().get(1),
+ seqs(2, 2),
+ ids(commitId2, commitId2),
+ files(FILE_C, FILE_D));
+
+ validateManifestEntries(snap2.allManifests().get(2),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D),
+ statuses(Status.EXISTING, Status.EXISTING, Status.EXISTING));
+
+ validateManifest(snap2.allManifests().get(2),
+ seqs(1, 1, 1),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D));
+ V2Assert.assertEquals("Snapshot sequence number should be 2", 2, snap2.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 2", 2, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
// validate that the metadata summary is correct when using appendManifest
Assert.assertEquals("Summary metadata should include 3 added files",
"3", readMetadata().currentSnapshot().summary().get("added-data-files"));
+
+ table.updateProperties()
Review comment:
It is the removed case in TestSequenceNumberForV2Table.java. Moved it to a new method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r438377910
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -251,7 +317,10 @@ public void testMergeWithExistingManifestAfterDelete() {
long baseId = base.currentSnapshot().snapshotId();
Assert.assertEquals("Should create 1 manifest for initial write",
1, base.currentSnapshot().allManifests().size());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, readMetadata().lastSequenceNumber());
Review comment:
Adding the sequence number asserts between the assertion about the number of manifests and validating a manifest makes it harder to read the test because lines that are related are now separated.
Can you move the snapshot and table metadata sequence number assertions to just after each commit?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r438453978
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A);
ManifestFile manifest2 = writeManifestWithName("FILE_C", FILE_C);
ManifestFile manifest3 = writeManifestWithName("FILE_D", FILE_D);
+ ManifestFile manifest4 = writeManifestWithName("FILE_B", FILE_B);
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
+ Snapshot snap1 = table.currentSnapshot();
+ long commitId1 = snap1.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 2 merged manifest for first write",
2, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap1.allManifests().get(0),
Review comment:
OK, I see the new validate method, will replace them with the new one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r438377012
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A);
ManifestFile manifest2 = writeManifestWithName("FILE_C", FILE_C);
ManifestFile manifest3 = writeManifestWithName("FILE_D", FILE_D);
+ ManifestFile manifest4 = writeManifestWithName("FILE_B", FILE_B);
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
+ Snapshot snap1 = table.currentSnapshot();
+ long commitId1 = snap1.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 2 merged manifest for first write",
2, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap1.allManifests().get(0),
+ seqs(1),
+ ids(commitId1),
+ files(FILE_A));
+ validateManifest(snap1.allManifests().get(1),
+ seqs(1, 1),
+ ids(commitId1, commitId1),
+ files(FILE_C, FILE_D));
+
+ V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap1.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
-
+ Snapshot snap2 = table.currentSnapshot();
+ long commitId2 = snap2.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 3 merged manifest for second write",
3, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap2.allManifests().get(0),
+ seqs(2),
+ ids(commitId2),
+ files(FILE_A));
+
+ validateManifest(snap2.allManifests().get(1),
+ seqs(2, 2),
+ ids(commitId2, commitId2),
+ files(FILE_C, FILE_D));
+
+ validateManifestEntries(snap2.allManifests().get(2),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D),
+ statuses(Status.EXISTING, Status.EXISTING, Status.EXISTING));
+
+ validateManifest(snap2.allManifests().get(2),
+ seqs(1, 1, 1),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D));
+ V2Assert.assertEquals("Snapshot sequence number should be 2", 2, snap2.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 2", 2, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
// validate that the metadata summary is correct when using appendManifest
Assert.assertEquals("Summary metadata should include 3 added files",
"3", readMetadata().currentSnapshot().summary().get("added-data-files"));
+
+ table.updateProperties()
Review comment:
Why were new commits added to this test case?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada closed pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
chenjunjiedada closed pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#issuecomment-663572907
@rdblue , this is holding for a while, could you please take another look? Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r467140291
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -183,31 +183,97 @@ public void testManifestMergeMinCount() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ V2Assert.assertEquals("Last sequence number should be 0", 0, readMetadata().lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A);
ManifestFile manifest2 = writeManifestWithName("FILE_C", FILE_C);
ManifestFile manifest3 = writeManifestWithName("FILE_D", FILE_D);
+ ManifestFile manifest4 = writeManifestWithName("FILE_B", FILE_B);
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
+ Snapshot snap1 = table.currentSnapshot();
+ long commitId1 = snap1.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 2 merged manifest for first write",
2, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap1.allManifests().get(0),
+ seqs(1),
+ ids(commitId1),
+ files(FILE_A));
+ validateManifest(snap1.allManifests().get(1),
+ seqs(1, 1),
+ ids(commitId1, commitId1),
+ files(FILE_C, FILE_D));
+
+ V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap1.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
table.newAppend()
.appendManifest(manifest)
.appendManifest(manifest2)
.appendManifest(manifest3)
.commit();
-
+ Snapshot snap2 = table.currentSnapshot();
+ long commitId2 = snap2.snapshotId();
+ base = readMetadata();
Assert.assertEquals("Should contain 3 merged manifest for second write",
3, readMetadata().currentSnapshot().allManifests().size());
+ validateManifest(snap2.allManifests().get(0),
+ seqs(2),
+ ids(commitId2),
+ files(FILE_A));
+
+ validateManifest(snap2.allManifests().get(1),
+ seqs(2, 2),
+ ids(commitId2, commitId2),
+ files(FILE_C, FILE_D));
+
+ validateManifestEntries(snap2.allManifests().get(2),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D),
+ statuses(Status.EXISTING, Status.EXISTING, Status.EXISTING));
+
+ validateManifest(snap2.allManifests().get(2),
+ seqs(1, 1, 1),
+ ids(commitId1, commitId1, commitId1),
+ files(FILE_A, FILE_C, FILE_D));
+ V2Assert.assertEquals("Snapshot sequence number should be 2", 2, snap2.sequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 2", 2, base.lastSequenceNumber());
+ V1Assert.assertEquals("Table should end with last-sequence-number 0", 0, base.lastSequenceNumber());
// validate that the metadata summary is correct when using appendManifest
Assert.assertEquals("Summary metadata should include 3 added files",
"3", readMetadata().currentSnapshot().summary().get("added-data-files"));
+
+ table.updateProperties()
Review comment:
What are the additional cases?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#issuecomment-670602196
Thanks for fixing the latest round of comments, @chenjunjiedada! I'm going to merge this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on a change in pull request #1101: Add sequence number assertions in merge append unit tests
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #1101:
URL: https://github.com/apache/iceberg/pull/1101#discussion_r466413940
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -75,17 +85,30 @@ public void testEmptyTableAppendManifest() throws IOException {
TableMetadata base = readMetadata();
Assert.assertNull("Should not have a current snapshot", base.currentSnapshot());
+ Assert.assertEquals("Last sequence number should be 0", 0, base.lastSequenceNumber());
ManifestFile manifest = writeManifest(FILE_A, FILE_B);
- Snapshot pending = table.newAppend()
+ table.newAppend()
.appendManifest(manifest)
- .apply();
+ .commit();
+
+ Snapshot committedSnapshot = table.currentSnapshot();
+ Assert.assertNotNull("Should create a snapshot", table.currentSnapshot());
+ V1Assert.assertEquals("Last sequence number should be 0", 0, table.ops().current().lastSequenceNumber());
+ V2Assert.assertEquals("Last sequence number should be 1", 1, table.ops().current().lastSequenceNumber());
- validateSnapshot(base.currentSnapshot(), pending, FILE_A, FILE_B);
+ validateSnapshot(base.currentSnapshot(), committedSnapshot, FILE_A, FILE_B);
Review comment:
Not needed. Removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org