rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1861325992
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -833,7 +833,17 @@ public List<ManifestFile> apply(TableMetadata base,
Snapshot snapshot) {
filterManager.filterManifests(
SnapshotUtil.schemaFor(base, targetBranch()),
snapshot != null ? snapshot.dataManifests(ops.io()) : null);
- long minDataSequenceNumber =
+
+ long minNewFileSequenceNumber =
Review Comment:
Okay, that's what I thought was happening, but I wanted to confirm. I think
it would be helpful to have a comment in the test that states what would have
failed.
So in the test, the file that was missing was the first delete file,
`FILE_A2_DELETES` that had sequence number 1, right? In the second rewrite,
`FILE_A` is rewritten as `FILE_A2`, but it is rewritten at the same sequence
number (1). The min sequence number of the other files is 3 for `FILE_D` after
the first rewrite.
Why is the first rewrite needed? Couldn't you reproduce the problem by
rewriting just `FILE_A` to `FILE_A2` after separately committing `FILE_B`? The
test case seems over-complicated to me so I think we need one that:
1. Reproduces the problem without this fix, and
2. Is a minimal set of steps to reproduce it
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
