rdblue commented on code in PR #5234:
URL: https://github.com/apache/iceberg/pull/5234#discussion_r1083103566
##########
core/src/test/java/org/apache/iceberg/TestOverwrite.java:
##########
@@ -164,40 +173,43 @@ public void testOverwriteFailsDelete() {
"Should reject commit with file not matching delete expression",
ValidationException.class,
"Cannot delete file where some, but not all, rows match filter",
- overwrite::commit);
+ () -> commit(table, overwrite, branch));
Assert.assertEquals(
- "Should not create a new snapshot", baseId,
table.currentSnapshot().snapshotId());
+ "Should not create a new snapshot", baseId, latestSnapshot(base,
branch).snapshotId());
}
@Test
public void testOverwriteWithAppendOutsideOfDelete() {
TableMetadata base = TestTables.readMetadata(TABLE_NAME);
- long baseId = base.currentSnapshot().snapshotId();
+ long baseId =
+ latestSnapshot(base, branch) == null ? -1 : latestSnapshot(base,
branch).snapshotId();
Review Comment:
Minor: prefer to keep ternary expressions as simple as possible. If you're
calling a method twice with the same arguments, you generally want to use a
variable:
```java
Snapshot latest = latestSnapshot(base, branch);
long baseId = latest == null ? -1 : latest.snapshotId();
```
It's also a bit odd that the current snapshot is always defined, but here we
handle the case where the branch does not yet exist. Why is that? Are we
committing to main and then implicitly branching somewhere?
--
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
