Skip to content

Core: Classify RowDelta with data files only as APPEND #14581

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

Merged
pvary merged 2 commits into from
Nov 18, 2025
Merged

Core: Classify RowDelta with data files only as APPEND #14581

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44bef22
Core: Classify RowDelta with data files only as APPEND
mxm Nov 13, 2025
dcd7e73
fixup! Add test case for adding data while removing data files
mxm Nov 17, 2025
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
4 changes: 4 additions & 0 deletions core/src/main/java/org/apache/iceberg/BaseRowDelta.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ protected BaseRowDelta self() {

@Override
protected String operation() {
if (addsDataFiles() && !addsDeleteFiles() && !deletesDataFiles()) {
Comment thread
mxm marked this conversation as resolved.
return DataOperations.APPEND;
}

if (addsDeleteFiles() && !addsDataFiles()) {
return DataOperations.DELETE;
}
Expand Down
24 changes: 24 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestRowDelta.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,25 @@ public void addOnlyDeleteFilesProducesDeleteOperation() {
assertThat(snap.deleteManifests(table.io())).hasSize(1);
}

@TestTemplate
public void addOnlyDataFilesProducesAppendOperation() {
SnapshotUpdate<?> rowDelta = table.newRowDelta().addRows(FILE_A).addRows(FILE_B);

@singhpk234 singhpk234 Nov 17, 2025

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.

how about adding a case simulating this as well : https://github.com/apache/iceberg/pull/14581/files#r2524111562

@pvary pvary Nov 17, 2025

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.

Good point!

@nastra nastra Nov 17, 2025

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.

+1 on adding this test case

@mxm mxm Nov 17, 2025

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.

Added. Please see dcd7e73.


commit(table, rowDelta, branch);
Snapshot snap = latestSnapshot(table, branch);
assertThat(snap.sequenceNumber()).isEqualTo(1);
assertThat(snap.operation()).isEqualTo(DataOperations.APPEND);
assertThat(snap.dataManifests(table.io())).hasSize(1);

validateManifest(
snap.dataManifests(table.io()).get(0),
dataSeqs(1L, 1L),
fileSeqs(1L, 1L),
ids(snap.snapshotId(), snap.snapshotId()),
files(FILE_A, FILE_B),
statuses(Status.ADDED, Status.ADDED));
}

@TestTemplate
public void testAddRemoveRows() {
SnapshotUpdate<?> rowDelta =
Expand Down Expand Up @@ -599,6 +618,7 @@ public void testOverwriteWithRemoveRows() {

long deltaSnapshotId = latestSnapshot(table, branch).snapshotId();
assertThat(latestSnapshot(table, branch).sequenceNumber()).isEqualTo(1);
assertThat(latestSnapshot(table, branch).operation()).isEqualTo(DataOperations.OVERWRITE);
assertThat(table.ops().current().lastSequenceNumber()).isEqualTo(1);

// overwriting by a filter will also remove delete files that match because all matching data
Expand Down Expand Up @@ -642,6 +662,7 @@ public void testReplacePartitionsWithRemoveRows() {

long deltaSnapshotId = latestSnapshot(table, branch).snapshotId();
assertThat(latestSnapshot(table, branch).sequenceNumber()).isEqualTo(1);
assertThat(latestSnapshot(table, branch).operation()).isEqualTo(DataOperations.OVERWRITE);
assertThat(table.ops().current().lastSequenceNumber()).isEqualTo(1);

// overwriting the partition will also remove delete files that match because all matching data
Expand Down Expand Up @@ -688,6 +709,7 @@ public void testDeleteByExpressionWithRemoveRows() {
branch);

assertThat(latestSnapshot(table, branch).sequenceNumber()).isEqualTo(1);
assertThat(latestSnapshot(table, branch).operation()).isEqualTo(DataOperations.OVERWRITE);
assertThat(table.ops().current().lastSequenceNumber()).isEqualTo(1);

// deleting with a filter will also remove delete files that match because all matching data
Expand Down Expand Up @@ -726,6 +748,7 @@ public void testDeleteDataFileWithRemoveRows() {

long deltaSnapshotId = latestSnapshot(table, branch).snapshotId();
assertThat(latestSnapshot(table, branch).sequenceNumber()).isEqualTo(1);
assertThat(latestSnapshot(table, branch).operation()).isEqualTo(DataOperations.OVERWRITE);
assertThat(table.ops().current().lastSequenceNumber()).isEqualTo(1);

// deleting a specific data file will not affect a delete file in v2 or less
Expand Down Expand Up @@ -786,6 +809,7 @@ public void testFastAppendDoesNotRemoveStaleDeleteFiles() {

long deltaSnapshotId = latestSnapshot(table, branch).snapshotId();
assertThat(latestSnapshot(table, branch).sequenceNumber()).isEqualTo(1);
assertThat(latestSnapshot(table, branch).operation()).isEqualTo(DataOperations.OVERWRITE);
assertThat(table.ops().current().lastSequenceNumber()).isEqualTo(1);

// deleting a specific data file will not affect a delete file
Expand Down