Skip to content

Core: Disallow adding and removing the same file in a rewrite files commit #17359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
amogh-jahagirdar wants to merge 3 commits into from
Closed

Core: Disallow adding and removing the same file in a rewrite files commit #17359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5f67fbd
Core: Disallow adding and removing the same file in a rewrite files c…
amogh-jahagirdar Jul 24, 2026
189f37b
Update flink test which uses same file in rewrite
amogh-jahagirdar Jul 26, 2026
852caa4
use existing utils
amogh-jahagirdar Jul 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions core/src/main/java/org/apache/iceberg/BaseRewriteFiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,5 +152,12 @@ private void validateReplacedAndAddedFiles() {
Preconditions.checkArgument(
deletesDeleteFiles() || !addsDeleteFiles(),
"Delete files to add must be empty because there's no delete file to be rewritten");

for (DataFile added : addedDataFiles()) {
Preconditions.checkArgument(
!replacedDataFiles.contains(added),
"Cannot add and delete the same file in the same rewrite: %s",
added.location());
}
Comment thread
amogh-jahagirdar marked this conversation as resolved.
Comment on lines +156 to +161

@dramaticlly dramaticlly Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if this is the right place to place the guard, if the goal is to prevent the same file being added and removed in the same snapshot? I think both overwrite-files and row-delta can be abused in a way to achieve the same goal.

On a separate note, knowing this produce incorrect result for the change tracking detection, sometimes such surgical operation can be helpful for stats backfill as to rectify a previously written file with incorrect column level stats, want to see if we shall encourage to use dedicated repair actions such as proposed in #10784

@amogh-jahagirdar amogh-jahagirdar Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was looking at this in the interim, I think you're right that Overwrite and Row Delta technically allow this and we should block them too.

On a separate note, knowing this produce incorrect result for the change tracking detection, sometimes such surgical operation can be helpful for stats backfill as to rectify a previously written file with incorrect column level stats, want to see if we shall encourage to use dedicated repair actions such as proposed in #10784

Yup! I think rewriting manifests (just like Repair would do) is the right way. After some thought, I don't think updating these operations to infer that add + remove = existing is the right thing to do because I think these APIs have largely been designed around the fact that added = "something new", and removed = "something that existed and we need to remove".

}
}
16 changes: 16 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestRewriteFiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,22 @@ public void testDeleteOnly() {
.hasMessage("Files to delete cannot be empty");
}

@TestTemplate
public void addingAndDeletingSameFileDisallowed() {

@dramaticlly dramaticlly Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might also need to fix a few other tests, namely testReadStreamWithSnapshotTypeRewriteDataFilesIgnoresReplaceMaxFilesAndRows because of

iceberg/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestStructuredStreamingRead3.java

Lines 1041 to 1051 in 6bfb510

for (Snapshot snapshot : it) {
if (snapshot.operation().equals(DataOperations.APPEND)) {
Iterable<DataFile> datafiles =
SnapshotChanges.builderFor(table).snapshot(snapshot).build().addedDataFiles();
for (DataFile datafile : datafiles) {
rewrite.addFile(datafile);
rewrite.deleteFile(datafile);
}
}
}
rewrite.commit();

assertThat(listManifestFiles()).isEmpty();

commit(table, table.newAppend().appendFile(FILE_A).appendFile(FILE_B), branch);

assertThatThrownBy(
() ->
apply(
table.newRewrite().rewriteFiles(Sets.newSet(FILE_A), Sets.newSet(FILE_A)),
branch))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(
"Cannot add and delete the same file in the same rewrite: " + FILE_A.location());
}

@TestTemplate
public void testDeleteWithDuplicateEntriesInManifest() {
assertThat(listManifestFiles()).isEmpty();
Expand Down
6 changes: 4 additions & 2 deletions .../flink/src/test/java/org/apache/iceberg/flink/maintenance/operator/TestMonitorSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.apache.iceberg.DataFile;
import org.apache.iceberg.DeleteFile;
import org.apache.iceberg.FileFormat;
import org.apache.iceberg.FileGenerationUtil;
import org.apache.iceberg.RewriteFiles;
import org.apache.iceberg.SnapshotChanges;
import org.apache.iceberg.Table;
Expand Down Expand Up @@ -310,10 +311,11 @@ void testSkipReplace() throws IOException {
// Create a DataOperations.REPLACE snapshot
DataFile dataFile =
SnapshotChanges.builderFor(table).build().addedDataFiles().iterator().next();
// Replace the file with a new file to produce a REPLACE snapshot
DataFile replacement = FileGenerationUtil.generateDataFile(table, null);
RewriteFiles rewrite = tableLoader.loadTable().newRewrite();
// Replace the file with itself for testing purposes
rewrite.deleteFile(dataFile);
rewrite.addFile(dataFile);
rewrite.addFile(replacement);
rewrite.commit();

// Check that the rewrite is ignored
Expand Down
6 changes: 4 additions & 2 deletions .../flink/src/test/java/org/apache/iceberg/flink/maintenance/operator/TestMonitorSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.apache.iceberg.DataFile;
import org.apache.iceberg.DeleteFile;
import org.apache.iceberg.FileFormat;
import org.apache.iceberg.FileGenerationUtil;
import org.apache.iceberg.RewriteFiles;
import org.apache.iceberg.SnapshotChanges;
import org.apache.iceberg.Table;
Expand Down Expand Up @@ -318,10 +319,11 @@ void testSkipReplace() throws IOException {
// Create a DataOperations.REPLACE snapshot
DataFile dataFile =
SnapshotChanges.builderFor(table).build().addedDataFiles().iterator().next();
// Replace the file with a new file to produce a REPLACE snapshot
DataFile replacement = FileGenerationUtil.generateDataFile(table, null);
RewriteFiles rewrite = tableLoader.loadTable().newRewrite();
// Replace the file with itself for testing purposes
rewrite.deleteFile(dataFile);
rewrite.addFile(dataFile);
rewrite.addFile(replacement);
rewrite.commit();

// Check that the rewrite is ignored
Expand Down
6 changes: 4 additions & 2 deletions .../flink/src/test/java/org/apache/iceberg/flink/maintenance/operator/TestMonitorSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.apache.iceberg.DataFile;
import org.apache.iceberg.DeleteFile;
import org.apache.iceberg.FileFormat;
import org.apache.iceberg.FileGenerationUtil;
import org.apache.iceberg.RewriteFiles;
import org.apache.iceberg.SnapshotChanges;
import org.apache.iceberg.Table;
Expand Down Expand Up @@ -318,10 +319,11 @@ void testSkipReplace() throws IOException {
// Create a DataOperations.REPLACE snapshot
DataFile dataFile =
SnapshotChanges.builderFor(table).build().addedDataFiles().iterator().next();
// Replace the file with a new file to produce a REPLACE snapshot
DataFile replacement = FileGenerationUtil.generateDataFile(table, null);
RewriteFiles rewrite = tableLoader.loadTable().newRewrite();
// Replace the file with itself for testing purposes
rewrite.deleteFile(dataFile);
rewrite.addFile(dataFile);
rewrite.addFile(replacement);
rewrite.commit();

// Check that the rewrite is ignored
Expand Down
Loading