You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "jasonf20 (via GitHub)" <gi...@apache.org> on 2024/08/18 17:33:45 UTC
[PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
jasonf20 opened a new pull request, #10962:
URL: https://github.com/apache/iceberg/pull/10962
This fixes a bug where delete files that are still required by the re-written file could be deleted prematurely.
This happened because `MergingSnapshotProducer` calculated `minDataSequenceNumber` in a way that could skip data sequence numbers that do exist in the final snapshot.
The filtered manifest list could exclude manifests of a certain sequence number because their data files were re-written. With specific orders of operations it was possible to skip the sequence number of manifests that were re-written.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1746200353
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
+ assumeThat(formatVersion)
+ .as("Sequence number is only supported in iceberg format v2 or later")
+ .isGreaterThan(1);
+ assertThat(listManifestFiles()).isEmpty();
+
+ commit(table, table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A2_DELETES), branch);
+
+ long firstRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_B).addDeletes(FILE_B2_DELETES),
+ branch);
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_C).addDeletes(FILE_C2_DELETES),
+ branch);
+
+ long secondRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table
+ .newRewrite()
+ .addFile(FILE_D)
+ .deleteFile(FILE_B)
+ .deleteFile(FILE_C)
+ .dataSequenceNumber(secondRewriteSequenceNumber),
+ branch);
+
+ TableMetadata base = readMetadata();
+ Snapshot baseSnap = latestSnapshot(base, branch);
+ long baseSnapshotId = baseSnap.snapshotId();
+
+ Comparator<ManifestFile> sequenceNumberOrdering =
+ new Comparator<>() {
+ @Override
+ public int compare(ManifestFile o1, ManifestFile o2) {
+ return (int) (o1.sequenceNumber() - o2.sequenceNumber());
+ }
+ };
+
+ // FILE_B2_DELETES and FILE_A2_DELETES should not be removed as the rewrite specifies
+ // `firstRewriteSequenceNumber`
+ // explicitly which is the same as that of A2_DELETES and before B2_DELETES
+
+ // Technically A1_DELETES could be removed since it's an equality delete and should apply on
Review Comment:
Sorry not sure I really follow the comment? There is no A1_DELETES....there's a FILE_A_DELETES but that's a positional delete. I don't see those referenced in the above operations.
I think it's true though that we should be able to drop equality deletes older than the minimum sequence number but that's already happening in the existing MergingSnapshotProducer check no? Don't think anything needs to distinguish there between equality and positional delete
##########
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 =
+ addedDataFiles().stream()
+ .filter(x -> x.dataSequenceNumber() != null && x.dataSequenceNumber() >= 0)
+ .mapToLong(ContentFile::dataSequenceNumber)
+ .reduce(
+ newDataFilesDataSequenceNumber != null
+ ? newDataFilesDataSequenceNumber
+ : base.nextSequenceNumber(),
+ Math::min);
Review Comment:
Do we actually need to iterate through the `addedDataFiles`?
If I understood the issue correctly, the problem is that it's possible for a user to commit a rewrite operation and specify an older data sequence number, and the current logic would drop delete files which actually need to still be referenced in the new commit since it's not considering the specified data file sequence number.
So I *think* all we would need to do here is keep the existing logic for determining minDataSequenceNumber and then also min that with the `newDataFilesDataSequenceNumber` if it's not null
```
long minNewDataSequenceNumber = <Existing logic>
if (newDataFilesDataSequenceNumber != null) {
minNewDataSequenceNumber = Math.min(minNewDataSequenceNumber, newDataFilesDataSequenceNumber);
}
```
##########
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 =
+ addedDataFiles().stream()
+ .filter(x -> x.dataSequenceNumber() != null && x.dataSequenceNumber() >= 0)
+ .mapToLong(ContentFile::dataSequenceNumber)
+ .reduce(
+ newDataFilesDataSequenceNumber != null
+ ? newDataFilesDataSequenceNumber
+ : base.nextSequenceNumber(),
+ Math::min);
Review Comment:
I tried the above and the new unit test still passes, though let me know if you see a flaw in my reasoning. cc @rdblue @aokolnychyi @findepi
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1859372481
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
+ assumeThat(formatVersion)
+ .as("Sequence number is only supported in iceberg format v2 or later")
+ .isGreaterThan(1);
+ assertThat(listManifestFiles()).isEmpty();
+
+ commit(table, table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A2_DELETES), branch);
+
+ long firstRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_B).addDeletes(FILE_B2_DELETES),
+ branch);
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_C).addDeletes(FILE_C2_DELETES),
+ branch);
+
+ long secondRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table
+ .newRewrite()
+ .addFile(FILE_D)
+ .deleteFile(FILE_B)
+ .deleteFile(FILE_C)
+ .dataSequenceNumber(secondRewriteSequenceNumber),
+ branch);
+
+ TableMetadata base = readMetadata();
+ Snapshot baseSnap = latestSnapshot(base, branch);
+ long baseSnapshotId = baseSnap.snapshotId();
+
+ Comparator<ManifestFile> sequenceNumberOrdering =
+ new Comparator<>() {
+ @Override
+ public int compare(ManifestFile o1, ManifestFile o2) {
+ return (int) (o1.sequenceNumber() - o2.sequenceNumber());
+ }
+ };
+
+ // FILE_B2_DELETES and FILE_A2_DELETES should not be removed as the rewrite specifies
+ // `firstRewriteSequenceNumber`
+ // explicitly which is the same as that of A2_DELETES and before B2_DELETES
+
+ // Technically A1_DELETES could be removed since it's an equality delete and should apply on
+ // data sequences strictly
+ // smaller, so it's no longer needed. However, MergingSnapshotProducer calls
+ // dropDeleteFilesOlderThan
+ // which doesn't consider if the file is an equality delete, if that API is changed the equality
+ // delete file could be dropped sooner
+ Snapshot pending =
+ apply(
+ table
+ .newRewrite()
+ .addFile(FILE_A2)
+ .deleteFile(FILE_A)
+ .dataSequenceNumber(firstRewriteSequenceNumber),
+ branch);
+
+ assertThat(pending.allManifests(table.io())).hasSize(6);
+
+ long pendingId = pending.snapshotId();
+ List<ManifestFile> manifestFiles =
+ pending.allManifests(table.io()).stream()
+ .sorted(sequenceNumberOrdering.reversed())
+ .collect(Collectors.toList());
+ ManifestFile newManifest = manifestFiles.get(0);
+ validateManifestEntries(newManifest, ids(pendingId), files(FILE_A2), statuses(ADDED));
+
+ assertThat(ManifestFiles.read(newManifest, FILE_IO).entries())
+ .allSatisfy(
+ entry -> assertThat(entry.dataSequenceNumber()).isEqualTo(firstRewriteSequenceNumber));
+ assertThat(newManifest.sequenceNumber()).isEqualTo(secondRewriteSequenceNumber + 2);
Review Comment:
Why not use the equivalent of `validateDeleteManifest` below where you can specify the data and file sequence numbers that should be validated? I don't see why this needs to validate the entries manually.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1861332491
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -924,6 +934,7 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
!= ManifestWriter
.UNASSIGNED_SEQ) // filter out unassigned in rewritten manifests
.reduce(base.lastSequenceNumber(), Math::min);
+ long minDataSequenceNumber = Math.min(minNewFileSequenceNumber, minExistingDataSequenceNumber);
Review Comment:
The logic for calculating the min sequence number is getting too long to embed here. I think it should be moved to a separate private method:
```java
private long minDataSequenceNumber() {
long minAddedDataSequenceNumber = addedDataFiles().stream()
.map(ContentFile::dataSequenceNumber)
.filter(Objects::nonNull)
.filter(seq -> seq >= 0)
.reduce(base.nextSequenceNumber(), Math::min);
long minExistingDataSequenceNumber =
filtered.stream()
.map(ManifestFile::minSequenceNumber)
.filter(
seq ->
seq
!= ManifestWriter
.UNASSIGNED_SEQ) // filter out unassigned in rewritten manifests
.reduce(base.lastSequenceNumber(), Math::min);
long minDataSequenceNumber = Math.min(minAddedDataSequenceNumber, minExistingDataSequenceNumber)
return Math.min(Math.min(minAddedDataSequenceNumber, minExistingDataSequenceNumber), newDataFilesDataSequenceNumber);
}
```
This also fixes the awkwardness of deciding whether to use `base.nextSequenceNumber()` or `newDataFilesDataSequenceNumber`.
I also agree with @amogh-jahagirdar that checking `addedDataFiles()` is not currently necessary, but it seems like a good idea to future-proof this issue in case we change the code again. This bug was likely introduced when we added `newDataFilesDataSequenceNumber`.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1859371399
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
Review Comment:
I've gone through this test and I think that the assertions are correct for the sequence of changes.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862492365
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
If that's the case then doesn't the last condition here do nothing since it can only reach this check if one of the earlier checks was true anyway:
```java
deletePaths.contains(file.location())
|| deleteFiles.contains(file)
|| dropPartitions.contains(file.specId(), file.partition())
|| (isDelete
&& entry.isLive()
&& entry.dataSequenceNumber() > 0
&& entry.dataSequenceNumber() < minSequenceNumber);
```
It seems like perhaps `dropDeleteFilesOlderThan` has no affect anymore (unless maybe `allDeletesReferenceManifests` gets set to false or something).
I think not removing by `minSequenceNumber` leaves undeleted delete files that just never get applied to any files at query time, so it's not the end of the world, but it does lead to some wasted storage and slightly longer scan planning times.
Assuming we want to keep this behaviour perhaps we should just not use `dropDeleteFilesOlderThan` anymore in `mergingSnapshotProducer` then?
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862410733
##########
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:
Took a look, @jasonf20 see my comment [here](https://github.com/apache/iceberg/pull/11131/files#r1815710103) , the change to not ropportunistically rewrite deletes in case of aged out deletes was actually intentional and I don't think we want to add that behavior back. Let me take a look to see how we can simplify the tests in this PR considering that manifests with aged out deletes won't be rewritten opportunistically until some other changes prompt a rewrite of the manifest
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1859362907
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
+ assumeThat(formatVersion)
+ .as("Sequence number is only supported in iceberg format v2 or later")
+ .isGreaterThan(1);
+ assertThat(listManifestFiles()).isEmpty();
+
+ commit(table, table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A2_DELETES), branch);
+
+ long firstRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_B).addDeletes(FILE_B2_DELETES),
+ branch);
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_C).addDeletes(FILE_C2_DELETES),
+ branch);
+
+ long secondRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table
+ .newRewrite()
+ .addFile(FILE_D)
+ .deleteFile(FILE_B)
+ .deleteFile(FILE_C)
+ .dataSequenceNumber(secondRewriteSequenceNumber),
+ branch);
+
+ TableMetadata base = readMetadata();
+ Snapshot baseSnap = latestSnapshot(base, branch);
+ long baseSnapshotId = baseSnap.snapshotId();
+
+ Comparator<ManifestFile> sequenceNumberOrdering =
+ new Comparator<>() {
+ @Override
+ public int compare(ManifestFile o1, ManifestFile o2) {
+ return (int) (o1.sequenceNumber() - o2.sequenceNumber());
+ }
+ };
+
+ // FILE_B2_DELETES and FILE_A2_DELETES should not be removed as the rewrite specifies
+ // `firstRewriteSequenceNumber`
+ // explicitly which is the same as that of A2_DELETES and before B2_DELETES
+
+ // Technically A1_DELETES could be removed since it's an equality delete and should apply on
+ // data sequences strictly
+ // smaller, so it's no longer needed. However, MergingSnapshotProducer calls
+ // dropDeleteFilesOlderThan
+ // which doesn't consider if the file is an equality delete, if that API is changed the equality
+ // delete file could be dropped sooner
+ Snapshot pending =
+ apply(
+ table
+ .newRewrite()
+ .addFile(FILE_A2)
+ .deleteFile(FILE_A)
+ .dataSequenceNumber(firstRewriteSequenceNumber),
+ branch);
+
+ assertThat(pending.allManifests(table.io())).hasSize(6);
+
+ long pendingId = pending.snapshotId();
+ List<ManifestFile> manifestFiles =
+ pending.allManifests(table.io()).stream()
+ .sorted(sequenceNumberOrdering.reversed())
+ .collect(Collectors.toList());
+ ManifestFile newManifest = manifestFiles.get(0);
+ validateManifestEntries(newManifest, ids(pendingId), files(FILE_A2), statuses(ADDED));
+
+ assertThat(ManifestFiles.read(newManifest, FILE_IO).entries())
Review Comment:
Why is `FILE_IO` used here, but `table.io()` is used elsewhere?
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2325605201
I'll take a pass over this PR tomorrow! thanks for your patience @jasonf20 !
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2309576940
@findepi I see it seems to be the flink tests specifically. I can't tell from the logs which test seems to be failing. Trying to run locally but I'm not sure how long that will take or if they will work locally. Anyway to get more information in the build about what failed?
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2308940613
@jasonf20 the CI looks red. Please try rebasing the PR and make sure it's green. I am not aware of any pre-existing flakiness that could cause 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2295933335
I'm not sure what the policy on supporting older version of Iceberg is, but this might be something that should be patched / hot-fixed to all supported versions.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2345965432
@amogh-jahagirdar If you'd like I can change the code to not iterate the files if that will make this quicker to merge, I think it's preferable to iterate them but it should be fine for now either way.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1864787713
##########
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:
@amogh-jahagirdar Responded [here](https://github.com/apache/iceberg/pull/10962#discussion_r1862492365).
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
If that's the case then doesn't the last condition here do nothing since it can only reach this check if one of the earlier checks was true anyway:
```java
deletePaths.contains(file.location())
|| deleteFiles.contains(file)
|| dropPartitions.contains(file.specId(), file.partition())
|| (isDelete
&& entry.isLive()
&& entry.dataSequenceNumber() > 0
&& entry.dataSequenceNumber() < minSequenceNumber);
```
It seems like perhaps `dropDeleteFilesOlderThan` has no affect anymore (unless maybe `allDeletesReferenceManifests` gets set to false or something).
I think not removing by `minSequenceNumber` leaves undeleted delete files that just never get applied to any files at query time, so it's not the end of the world, but it does lead to some wasted storage and slightly longer scan planning times.
Assuming we want to keep this behaviour perhaps we should just not use `dropDeleteFilesOlderThan` anymore in `mergingSnapshotProducer` then?
We can try getting a minimal test working by doing some actual delete or something from a shared manifest. But it seems like `dropDeleteFilesOlderThan` is not exactly doing anything right now.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2308725755
@findepi sorry for the ping. This causes duplicate data. Though in rare orders of operation, but I still think it's important to merge. Anyway to move this forward?
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1746659034
##########
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 =
+ addedDataFiles().stream()
+ .filter(x -> x.dataSequenceNumber() != null && x.dataSequenceNumber() >= 0)
+ .mapToLong(ContentFile::dataSequenceNumber)
+ .reduce(
+ newDataFilesDataSequenceNumber != null
+ ? newDataFilesDataSequenceNumber
+ : base.nextSequenceNumber(),
+ Math::min);
Review Comment:
Currently _I think_ ('m also not 100% sure) you are right as there isn't an API that would allow setting a specific data file to a SN smaller than `newDataFilesDataSequenceNumber`. But I think that's just a side affect of the current operations inheriting from `MergingSnapshotProducer`. A new inheritor of `MergingSnapshotProducer` could simply be using `minNewDataSequenceNumber` as a default and pass in files with explicit SNs on top.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2310004973
The build times out. Can it be that something takes more time as a result of the changes being introduced here?
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1860941952
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
+ assumeThat(formatVersion)
+ .as("Sequence number is only supported in iceberg format v2 or later")
+ .isGreaterThan(1);
+ assertThat(listManifestFiles()).isEmpty();
+
+ commit(table, table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A2_DELETES), branch);
+
+ long firstRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_B).addDeletes(FILE_B2_DELETES),
+ branch);
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_C).addDeletes(FILE_C2_DELETES),
Review Comment:
I don't remember, I think it may just be a mistake in the test. I removed the extra copies.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2310331550
@findepi Looks good now :)
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862157059
##########
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:
@rdblue I had some time to look at this today and have managed to reduce the spec to the minimal required.
I pushed a new commit that fixes a bug introduces in the [PR mentioned above](https://github.com/apache/iceberg/pull/11131/files). After fixing this bug my new simplified test reproduces the issue.
@amogh-jahagirdar Would appreciate if you can look at the first commit since it's related to your PR. 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862406185
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
I don't think we want to add this, I intentionally removed the behavior to rewrite a manifest in the case it only had aged out deletes, see here for a more detailed description https://github.com/apache/iceberg/pull/11131/files#r1815710103.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1860928881
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
+ assumeThat(formatVersion)
+ .as("Sequence number is only supported in iceberg format v2 or later")
+ .isGreaterThan(1);
+ assertThat(listManifestFiles()).isEmpty();
+
+ commit(table, table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A2_DELETES), branch);
+
+ long firstRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_B).addDeletes(FILE_B2_DELETES),
+ branch);
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_C).addDeletes(FILE_C2_DELETES),
+ branch);
+
+ long secondRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table
+ .newRewrite()
+ .addFile(FILE_D)
+ .deleteFile(FILE_B)
+ .deleteFile(FILE_C)
+ .dataSequenceNumber(secondRewriteSequenceNumber),
+ branch);
+
+ TableMetadata base = readMetadata();
+ Snapshot baseSnap = latestSnapshot(base, branch);
+ long baseSnapshotId = baseSnap.snapshotId();
+
+ Comparator<ManifestFile> sequenceNumberOrdering =
+ new Comparator<>() {
+ @Override
+ public int compare(ManifestFile o1, ManifestFile o2) {
+ return (int) (o1.sequenceNumber() - o2.sequenceNumber());
+ }
+ };
+
+ // FILE_B2_DELETES and FILE_A2_DELETES should not be removed as the rewrite specifies
+ // `firstRewriteSequenceNumber`
+ // explicitly which is the same as that of A2_DELETES and before B2_DELETES
+
+ // Technically A1_DELETES could be removed since it's an equality delete and should apply on
+ // data sequences strictly
+ // smaller, so it's no longer needed. However, MergingSnapshotProducer calls
+ // dropDeleteFilesOlderThan
+ // which doesn't consider if the file is an equality delete, if that API is changed the equality
+ // delete file could be dropped sooner
+ Snapshot pending =
+ apply(
+ table
+ .newRewrite()
+ .addFile(FILE_A2)
+ .deleteFile(FILE_A)
+ .dataSequenceNumber(firstRewriteSequenceNumber),
+ branch);
+
+ assertThat(pending.allManifests(table.io())).hasSize(6);
+
+ long pendingId = pending.snapshotId();
+ List<ManifestFile> manifestFiles =
+ pending.allManifests(table.io()).stream()
+ .sorted(sequenceNumberOrdering.reversed())
+ .collect(Collectors.toList());
+ ManifestFile newManifest = manifestFiles.get(0);
+ validateManifestEntries(newManifest, ids(pendingId), files(FILE_A2), statuses(ADDED));
+
+ assertThat(ManifestFiles.read(newManifest, FILE_IO).entries())
Review Comment:
Fixed
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1860926978
##########
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:
@rdblue Sure, the filtered manifest list could exclude manifests of a certain sequence number because their data files were re-written.
For example the current re-write replaces the manifest that contained the min sequence number, but preserves it. The `filtered` list won't contain that manifest, but the new manifest created will still have that same sequence number if the rewrite kept the sequence number small enough.
So the existing logic here that just took the the min from the filtered could miss the actual minimum since the manifest containing the actual minimum was re-written as part of this apply. So we need to consider also the minimum SN that will be present in the manifest that will get created with this operation.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1860955258
##########
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:
One note on this. Since I created this PR, this [PR](https://github.com/apache/iceberg/pull/11131/files) changed the way the `filtered` manifest list is calculated and I don't believe my test reproduces the issue anymore if you undo the code fix.
However, I think the fix is still correct and these the minSequenceNumber of the current operation should still be considered in addition to the filtered list.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1859256224
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
+ assumeThat(formatVersion)
+ .as("Sequence number is only supported in iceberg format v2 or later")
+ .isGreaterThan(1);
+ assertThat(listManifestFiles()).isEmpty();
+
+ commit(table, table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A2_DELETES), branch);
+
+ long firstRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_B).addDeletes(FILE_B2_DELETES),
+ branch);
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_C).addDeletes(FILE_C2_DELETES),
Review Comment:
Why are there additional copies of `FILE_B` in this commit and the one before it? Won't those be removed in the rewrite?
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1860926978
##########
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:
@rdblue Sure, the filtered manifest list could exclude manifests of a certain sequence number because their data files were re-written.
For example the current re-write replaces the manifest that contained the min sequence number, but preserves that file at that SN. The `filtered` list won't contain that manifest, but the new manifest created will still have that same sequence number if the rewrite kept the sequence number small enough.
So the existing logic here that just took the the min from the filtered could miss the actual minimum since the manifest containing the actual minimum was re-written as part of this apply. So we need to consider also the minimum SN that will be present in the manifest that will get created with this operation.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862410733
##########
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:
Took a look, @jasonf20 see my comment [here](https://github.com/apache/iceberg/pull/11131/files#r1815710103) , the change to not opportunistically rewrite deletes in case of aged out deletes was actually intentional and I don't think we want to add that behavior back. Let me take a look to see how we can simplify the tests in this PR considering that manifests with only aged out deletes won't be rewritten opportunistically until some other changes prompt a rewrite of the manifest
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862410733
##########
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:
Took a look, @jasonf20 see my comment [here](https://github.com/apache/iceberg/pull/11131/files#r1815710103) , the change to not opportunistically rewrite deletes in case of aged out deletes was actually intentional and I don't think we want to add that behavior back. Let me take a look to see how we can simplify the tests in this PR considering that manifests with aged out deletes won't be rewritten opportunistically until some other changes prompt a rewrite of the manifest
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862406185
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
I don't think we want to add this, I intentionally removed the behavior to opportunistically rewrite a manifest in the case it only had aged out deletes, see here for a more detailed description https://github.com/apache/iceberg/pull/11131/files#r1815710103.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1859261982
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -108,6 +108,14 @@ public class TestBase {
.withPartitionPath("data_bucket=1") // easy way to set partition data for now
.withRecordCount(1)
.build();
+ static final DeleteFile FILE_B2_DELETES =
Review Comment:
I think a better name would be `FILE_B_EQ_DELETES` because I think that's how you're using 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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2310685377
Thanks for fixing the build!
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862410733
##########
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:
Took a look, @jasonf20 see my comment [here](https://github.com/apache/iceberg/pull/11131/files#r1815710103) , the change to not opportunistically rewrite deletes in case of a manifest only having aged out deletes was actually intentional and I don't think we want to add that behavior back. Let me take a look to see how we can simplify the tests in this PR considering that manifests with only aged out deletes won't be rewritten opportunistically until some other changes prompt a rewrite of the manifest
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862410733
##########
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:
Took a look, @jasonf20 see my comment [here](https://github.com/apache/iceberg/pull/11131/files#r1815710103) , the change to not rewrite deletes in case of a manifest only having aged out deletes was actually intentional and I don't think we want to add that behavior back. Let me take a look to see how we can simplify the tests in this PR considering that manifests with only aged out deletes will only be rewritten opportunistically if some other changes prompt a rewrite of the manifest
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1874771178
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
As a side note I think it's better to make sure `dropDeleteFilesOlderThan` does work. We have encountered tables that have many old EQ deleted files that were compacted and no longer apply to any data files, but were not removed from the table's manifests.
Note that some manifests contain only irrelevant deletes and would not opportunistically get re-written for other purposes. Also, ReWriteManifests currently ignores delete files.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1880327776
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
Perhaps it can be added as a table property. As an anecdote I have encountered tables with frequent updates that have 100K + inactive delete files in their manifests. And since Spark isn't used in this env performing a cleanup is simple.
I think the above issue should be addressed, but perhaps we got a bit side tracked in the context of this bug. How would you like to proceed here. This is still a valid bug fix, can you think of a trick to make the test reproduce the issue easily without reverting the change to `dropDeleteFilesOlderThan`? If not can we merge it regardless, it's still a valid fix.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2467006719
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862492365
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
If that's the case then doesn't the last condition here do nothing since it can only reach this check if one of the earlier checks was true anyway:
```java
deletePaths.contains(file.location())
|| deleteFiles.contains(file)
|| dropPartitions.contains(file.specId(), file.partition())
|| (isDelete
&& entry.isLive()
&& entry.dataSequenceNumber() > 0
&& entry.dataSequenceNumber() < minSequenceNumber);
```
It seems like perhaps `dropDeleteFilesOlderThan` has no affect anymore (unless `allDeletesReferenceManifests` gets set to false or something).
I think not removing by `minSequenceNumber` leaves undeleted delete files that just never get applied to any files at query time, so it's not the end of the world, but it does lead to some wasted storage and slightly longer scan planning times.
Assuming we want to keep this behaviour perhaps we should just not use `dropDeleteFilesOlderThan` anymore in `mergingSnapshotProducer` then?
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1859381182
##########
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:
@jasonf20 can you summarize what the problem was? I didn't see it in the PR description. These changes look correct to me but it would be helpful to know where the issue was.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1861730623
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -924,6 +934,7 @@ public List<ManifestFile> apply(TableMetadata base, Snapshot snapshot) {
!= ManifestWriter
.UNASSIGNED_SEQ) // filter out unassigned in rewritten manifests
.reduce(base.lastSequenceNumber(), Math::min);
+ long minDataSequenceNumber = Math.min(minNewFileSequenceNumber, minExistingDataSequenceNumber);
Review Comment:
Sure looks good. I extracted it to a method and pushed.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1877273362
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
Sorry for the delay, yes I believe you are right that the last condition specifically at https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/ManifestFilterManager.java#L427 is redundant, we can remove that! There is no longer any need to rely on that condition because the only time we'd hit that point is the manifest needs to be rewritten for correctness based on the other criteria. Then when the manifest does get rewritten if there happen to be aged out deletes, we will rewrite them here https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/ManifestFilterManager.java#L475
OK so breaking down the pros/cons for eagerly removing aged out deletes during commit where a manifest *only* has aged out deletes:
The pro of eager removal of aged out deletes is that space consumed by metadata is reduced, and also planning times are at least slightly reduced.
The downside of eager removal is that even if a manifest does not need to be rewritten, the commit process is incurring additional latency and risk of failure (the IO involved) for writing out a new manifest.
IMO I feel like the upside for reclaiming some metadata storage/reducing plan times, doesn't justify the risk of failure/additional latency being incurred by the process on the commit path. Maybe in the most extreme case where some large percentage of manifests _only_ have aged out deletes, is it worth it?
I think you brought up good point though on rewrite manifests, the Java API itself will just retain all the delete manifests but I do think the spark action does support rewriting delete manifests https://github.com/apache/iceberg/blob/main/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteManifestsSparkAction.java#L179
Perhaps it would be better for rewrite manifests to remove the aged out deletes? At that point the work for removing the aged out deletes is more intentional in the context of table maintenance and not on the general commit path for `MergingSnapshotProducer` implementations? I'll need to think more on 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1941874030
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
I agree with Amogh here. Unnecessary cleanup is probably not a good idea in the core library when it requires rewriting manifests that would otherwise not be modified.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2310067114
@findepi
Locally the test `TestChangeLogTable` seems to run forever. Perhaps it's waiting for an event that is not published anymore. It seems to hang on the line
```
sql("INSERT INTO %s SELECT * FROM %s", tableName, SOURCE_TABLE);
```
Debugging that I found that it reached the code I changed with `addedDataFiles` that had a data sequence number of `-1` (not null). I don't know why that would cause the hang, but I filtered out those values and locally it seems to run now.
I'm pushing a new version.
I can't tell how my change would cause this line to hang, perhaps increased threading during
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2310686373
LGTM but someone more experienced with snapshot producers should also take a look.
@Fokko who would be the best person to take the second look?
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862406185
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
I don't think we want to add this, I intentionally removed the behavior to rewrite a manifest in the case it only had aged out deletes, see here for a more detailed description https://github.com/apache/iceberg/pull/11131/files#r1815710103.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1859255414
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
+ assumeThat(formatVersion)
+ .as("Sequence number is only supported in iceberg format v2 or later")
+ .isGreaterThan(1);
+ assertThat(listManifestFiles()).isEmpty();
+
+ commit(table, table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A2_DELETES), branch);
+
+ long firstRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_B).addDeletes(FILE_B2_DELETES),
+ branch);
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_C).addDeletes(FILE_C2_DELETES),
+ branch);
+
+ long secondRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table
+ .newRewrite()
+ .addFile(FILE_D)
+ .deleteFile(FILE_B)
+ .deleteFile(FILE_C)
+ .dataSequenceNumber(secondRewriteSequenceNumber),
+ branch);
+
+ TableMetadata base = readMetadata();
+ Snapshot baseSnap = latestSnapshot(base, branch);
+ long baseSnapshotId = baseSnap.snapshotId();
+
+ Comparator<ManifestFile> sequenceNumberOrdering =
+ new Comparator<>() {
+ @Override
+ public int compare(ManifestFile o1, ManifestFile o2) {
+ return (int) (o1.sequenceNumber() - o2.sequenceNumber());
+ }
+ };
+
+ // FILE_B2_DELETES and FILE_A2_DELETES should not be removed as the rewrite specifies
+ // `firstRewriteSequenceNumber`
+ // explicitly which is the same as that of A2_DELETES and before B2_DELETES
+
+ // Technically A1_DELETES could be removed since it's an equality delete and should apply on
+ // data sequences strictly
+ // smaller, so it's no longer needed. However, MergingSnapshotProducer calls
+ // dropDeleteFilesOlderThan
+ // which doesn't consider if the file is an equality delete, if that API is changed the equality
+ // delete file could be dropped sooner
Review Comment:
Looks like this comment is not correctly formatted. Can you fix it please?
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1859257408
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
+ assumeThat(formatVersion)
+ .as("Sequence number is only supported in iceberg format v2 or later")
+ .isGreaterThan(1);
+ assertThat(listManifestFiles()).isEmpty();
+
+ commit(table, table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A2_DELETES), branch);
+
+ long firstRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_B).addDeletes(FILE_B2_DELETES),
+ branch);
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_C).addDeletes(FILE_C2_DELETES),
+ branch);
+
+ long secondRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table
+ .newRewrite()
+ .addFile(FILE_D)
+ .deleteFile(FILE_B)
+ .deleteFile(FILE_C)
+ .dataSequenceNumber(secondRewriteSequenceNumber),
+ branch);
+
+ TableMetadata base = readMetadata();
+ Snapshot baseSnap = latestSnapshot(base, branch);
+ long baseSnapshotId = baseSnap.snapshotId();
+
+ Comparator<ManifestFile> sequenceNumberOrdering =
Review Comment:
You can use `Comparator.comparingLong(ManifestFile::sequenceNumber)`
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1860928230
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
+ assumeThat(formatVersion)
+ .as("Sequence number is only supported in iceberg format v2 or later")
+ .isGreaterThan(1);
+ assertThat(listManifestFiles()).isEmpty();
+
+ commit(table, table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A2_DELETES), branch);
+
+ long firstRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_B).addDeletes(FILE_B2_DELETES),
+ branch);
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_C).addDeletes(FILE_C2_DELETES),
+ branch);
+
+ long secondRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table
+ .newRewrite()
+ .addFile(FILE_D)
+ .deleteFile(FILE_B)
+ .deleteFile(FILE_C)
+ .dataSequenceNumber(secondRewriteSequenceNumber),
+ branch);
+
+ TableMetadata base = readMetadata();
+ Snapshot baseSnap = latestSnapshot(base, branch);
+ long baseSnapshotId = baseSnap.snapshotId();
+
+ Comparator<ManifestFile> sequenceNumberOrdering =
+ new Comparator<>() {
+ @Override
+ public int compare(ManifestFile o1, ManifestFile o2) {
+ return (int) (o1.sequenceNumber() - o2.sequenceNumber());
+ }
+ };
+
+ // FILE_B2_DELETES and FILE_A2_DELETES should not be removed as the rewrite specifies
+ // `firstRewriteSequenceNumber`
+ // explicitly which is the same as that of A2_DELETES and before B2_DELETES
+
+ // Technically A1_DELETES could be removed since it's an equality delete and should apply on
+ // data sequences strictly
+ // smaller, so it's no longer needed. However, MergingSnapshotProducer calls
+ // dropDeleteFilesOlderThan
+ // which doesn't consider if the file is an equality delete, if that API is changed the equality
+ // delete file could be dropped sooner
+ Snapshot pending =
+ apply(
+ table
+ .newRewrite()
+ .addFile(FILE_A2)
+ .deleteFile(FILE_A)
+ .dataSequenceNumber(firstRewriteSequenceNumber),
+ branch);
+
+ assertThat(pending.allManifests(table.io())).hasSize(6);
+
+ long pendingId = pending.snapshotId();
+ List<ManifestFile> manifestFiles =
+ pending.allManifests(table.io()).stream()
+ .sorted(sequenceNumberOrdering.reversed())
+ .collect(Collectors.toList());
+ ManifestFile newManifest = manifestFiles.get(0);
+ validateManifestEntries(newManifest, ids(pendingId), files(FILE_A2), statuses(ADDED));
+
+ assertThat(ManifestFiles.read(newManifest, FILE_IO).entries())
+ .allSatisfy(
+ entry -> assertThat(entry.dataSequenceNumber()).isEqualTo(firstRewriteSequenceNumber));
+ assertThat(newManifest.sequenceNumber()).isEqualTo(secondRewriteSequenceNumber + 2);
Review Comment:
I think I just missed that function. I changed 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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1861737685
##########
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:
All the operations were required to reproduce the issue, but It's been a while so I don't remember the specifics. I could try and get back into it and see if I can create a new minimal test. But I'm not sure exactly when I will be free to do that.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1862410733
##########
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:
Took a look, @jasonf20 see my comment [here](https://github.com/apache/iceberg/pull/11131/files#r1815710103) , the change to not rewrite deletes in case of aged out deletes was actually intentional and I don't think we want to add that behavior back. Let me take a look to see how we can simplify the tests in this PR considering that manifests with aged out deletes won't be rewritten opportunistically until some other changes prompt a rewrite of the manifest
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1880327776
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
Perhaps it can be added as a table property. As an anecdote I have encountered tables with frequent updates that have 100K + inactive delete files in their manifests. And since Spark isn't used in this env performing a cleanup isn't simple.
I think the above issue should be addressed, but perhaps we got a bit side tracked in the context of this bug. How would you like to proceed here. This is still a valid bug fix, can you think of a trick to make the test reproduce the issue easily without reverting the change to `dropDeleteFilesOlderThan`? If not can we merge it regardless, it's still a valid fix.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2467702044
I'll send a message about this in [dev@iceberg.apache.org](mailto:dev@iceberg.apache.org)
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1746650336
##########
core/src/test/java/org/apache/iceberg/TestRewriteFiles.java:
##########
@@ -384,6 +386,116 @@ public void testRewriteDataAndAssignOldSequenceNumber() {
assertThat(listManifestFiles()).hasSize(4);
}
+ @TestTemplate
+ public void testRewriteDataAndAssignOldSequenceNumbersShouldNotDropDeleteFiles() {
+ assumeThat(formatVersion)
+ .as("Sequence number is only supported in iceberg format v2 or later")
+ .isGreaterThan(1);
+ assertThat(listManifestFiles()).isEmpty();
+
+ commit(table, table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A2_DELETES), branch);
+
+ long firstRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_B).addDeletes(FILE_B2_DELETES),
+ branch);
+ commit(
+ table,
+ table.newRowDelta().addRows(FILE_B).addRows(FILE_C).addDeletes(FILE_C2_DELETES),
+ branch);
+
+ long secondRewriteSequenceNumber = latestSnapshot(table, branch).sequenceNumber();
+
+ commit(
+ table,
+ table
+ .newRewrite()
+ .addFile(FILE_D)
+ .deleteFile(FILE_B)
+ .deleteFile(FILE_C)
+ .dataSequenceNumber(secondRewriteSequenceNumber),
+ branch);
+
+ TableMetadata base = readMetadata();
+ Snapshot baseSnap = latestSnapshot(base, branch);
+ long baseSnapshotId = baseSnap.snapshotId();
+
+ Comparator<ManifestFile> sequenceNumberOrdering =
+ new Comparator<>() {
+ @Override
+ public int compare(ManifestFile o1, ManifestFile o2) {
+ return (int) (o1.sequenceNumber() - o2.sequenceNumber());
+ }
+ };
+
+ // FILE_B2_DELETES and FILE_A2_DELETES should not be removed as the rewrite specifies
+ // `firstRewriteSequenceNumber`
+ // explicitly which is the same as that of A2_DELETES and before B2_DELETES
+
+ // Technically A1_DELETES could be removed since it's an equality delete and should apply on
Review Comment:
I meant to say FILE_A2_DELETES. The existing snapshot producer check drops files older than the minimum sequence number, only if their sequence number is strictly smaller. So it leaves equality deletes in place that have a SN equal to the SN of the min SN. While equality deletes are only applied on data files with SNs that are strictly smaller than the delete file's SN. So you can actually delete equality delete files that are the same sequence number as the minimum sequence number of the new snapshot.
In an even broader sense FILE_A2_DELETES could even be ignored completely when doing the initial commit here since there are no data files before it since this is the first commit. So effectively it doesn't do anything.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Fixed dropping delete files that are still required [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#issuecomment-2298921650
Flink jobs all failed with timeout after 6h (https://github.com/apache/iceberg/actions/runs/10442521766/job/28914906512?pr=10962)
I did restart them.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1874771178
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
As a side note I think it's better to make sure `dropDeleteFilesOlderThan` does work. We have encountered tables that have many old EQ deleted files that were compacted and no longer apply to any data files, but were not removed from the table's manifests.
Note that some manifests contain only irrelevant deletes and would not opportunistically get re-written for other purposes. Also, ReWriteManifests currently ignores delete manifests.
--
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-unsubscribe@iceberg.apache.org
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
Re: [PR] Core/RewriteFiles: Duplicate Data Bug - Fixed dropping delete files that are still required [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10962:
URL: https://github.com/apache/iceberg/pull/10962#discussion_r1874771178
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -363,6 +363,10 @@ private ManifestFile filterManifest(
}
private boolean canContainDeletedFiles(ManifestFile manifest, boolean trustManifestReferences) {
+ if (manifest.minSequenceNumber() > 0 && manifest.minSequenceNumber() < minSequenceNumber) {
+ return true;
+ }
Review Comment:
As a side note I think it's better to make sure `dropDeleteFilesOlderThan` does work. We have encountered tables that have many old EQ deleted files that were compacted and no longer apply to any data files, but were not removed from the table's manifests.
--
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-unsubscribe@iceberg.apache.org
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