Core, Spark: Use 'delete' if RowDelta only has delete files by nastra · Pull Request #10123 · apache/iceberg

nastra · 2024-04-11T14:27:22Z

Resolves #10122

amogh-jahagirdar · 2024-04-11T15:33:04Z

core/src/main/java/org/apache/iceberg/BaseRowDelta.java


  @Override
  protected String operation() {
+    if (!addsDataFiles() && addsDeleteFiles()) {


If I understand right, wouldn't this condition need to be

if (!addsDataFiles() && (addsDeleteFiles() || deletesDataFiles())

Basically I think for a metadata only delete we'd want to make sure the operation is delete

yes you're absolutely right. Fixed it. This also needs some more tests, hence why it's WIP

@amogh-jahagirdar actually I think if (!addsDataFiles() && addsDeleteFiles()) is correct as the RowDelta API only provides addRows(DataFile) and addDeletes(DeleteFile). deletesDataFiles() would apply for the RewriteFiles API.

Ah true, but doesn't the same principle apply for RewriteFiles then? I think technically someone can use RewriteFiles to perform just a delete.

table.newRewrite().deleteFile(file)).commit()

It's awkward since they could just do a deleteFile but I think it's still possible, and we'd still want to produce the DELETE operation instead of a REPLACE. It seems like we should have a check in RewriteFiles as well.

@aokolnychyi I agree that we should use replace when it's the same logical set of data but I think the issue is on what RewriteFiles actually enforces currently. It doesn't do any validation on not only having deletes, and I wouldn't expect a logical comparison since that requires a data comparison which isn't feasible.

I wrote the following test in TestRewriteFiles:

@TestTemplate public void testDelete() { commit(table, table.newAppend().appendFile(FILE_A).appendFile(FILE_B), branch); table.newRewrite().deleteFile(FILE_A).toBranch(branch).commit(); table.currentSnapshot(); }

and the operation commits successfully with a REPLACE operation. I know it's strange to use this API (and goes against what's in the JavaDoc) for just deleting a file but as of today it's technically possible and would lead to producing a snapshot operation with the incorrect REPLACE operation which can lead to issues such as the incremental processing case.

There's 2 ways to address this incorrectness that I can think of:

1.) Fail at the time of committing if there's only deletes or only additions. If this is the case then we know that the data cannot possibly be logically the same since there'd essentially be a net gain or net loss in data. I'm not sure if there's any other cases but this seems like a reasonable check which doesn't require actually comparing the data.
2.) Don't fail (to preserve the API behavior) and rather at the time of committing, if it's only a delete or only an append, use the correct API to perform the operation with the right summary.

I'm leaning towards 1, since the API docs already makes it clear that from an API perspective RewriteFiles should only be used when it's logically the same. If we know for a fact that it's not locally the same table with a simple check, we can enforce that.

I see we have some validation in BaseRewriteFiles#validateReplacedAndAddedFiles, I think we're just missing this case. I'll raise a PR to show what I mean

The use case you highlight is a valid one if FILE_A has all records removed with deletes. Then removing FILE_A has no impact on the table state.

That being said, we should definitely check if the validation is missing any edge case. Thanks for looking into that!

Cool, thanks for explaining! This issue is ultimately orthogonal to the one that is being solved in this PR so we can take that separately.

nastra · 2024-04-12T10:13:40Z

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

            deleteRowSchema);

-    table.newRowDelta().addDeletes(eqDeletes).commit();
+    DataFile dataFile =


the test is specifically testing for OVERWRITE, hence I'm adding a data file here

nastra · 2024-04-12T14:16:45Z

spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestDelete.java

  }

+  @TestTemplate
+  public void deleteSingleRecordProducesDeleteOperation() {


something seems to be off with subclasses of TestDelete. The test produces different results on each run when executing ./gradlew :iceberg-spark:iceberg-spark-extensions-3.5_2.12:test --tests "org.apache.iceberg.spark.extensions.TestCopyOnWriteDelete" --tests "org.apache.iceberg.spark.extensions.TestMergeOnReadDelete" vs when running TestCopyOnWriteDelete or TestMergeOnReadDelete within the IDE

turns out this was because of the randomness of configuring adaptive query execution:

iceberg/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/ExtensionsTestBase.java

Line 56 in d1e24f4

SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), String.valueOf(RANDOM.nextBoolean()))

I'll port this test to Spark 3.3/3.4 once CI passes

I may have an explanation here.

nastra · 2024-04-12T14:24:11Z

...sions/src/test/java/org/apache/iceberg/spark/extensions/SparkRowLevelOperationsTestBase.java

-    assertThat(actual)
-        .as("Snapshot property " + property + " has unexpected value.")
-        .isEqualTo(expectedValue);
+    if (null == expectedValue) {


these changes will show the content of the snapshot summary in case the assertion fails

nastra · 2024-04-15T10:15:57Z

...sions/src/test/java/org/apache/iceberg/spark/extensions/SparkRowLevelOperationsTestBase.java

-    String actual = snapshot.summary().get(property);
-    Assert.assertEquals(
-        "Snapshot property " + property + " has unexpected value.", expectedValue, actual);
+    if (null == expectedValue) {


these changes will show the content of the snapshot summary in case the assertion fails

aokolnychyi · 2024-04-15T18:37:20Z

spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestDelete.java

+
+    Snapshot currentSnapshot = table.currentSnapshot();
+
+    if (Boolean.parseBoolean(spark.conf().get(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key()))) {


I recommend using append methods in SparkRowLevelOperationsTestBase to make sure the 3 records are added as a single file (I mean versions of append that accept json). Otherwise, you have to check for AQE and this logic is very fragile overall.

good point, I've updated that to use append methods

aokolnychyi · 2024-04-15T18:43:01Z

core/src/main/java/org/apache/iceberg/BaseRowDelta.java

+    if (!addsDataFiles() && addsDeleteFiles()) {
+      return DataOperations.DELETE;
+    }
+


Optional: My personal preference would be to have the negation as the second argument (or even offer a new method in the parent with the negated meaning) and combine the two return statements into one. It is completely up to you, though.

if (addsDeleteFiles() && !addsDataFiles()) { return DataOperations.DELETE; } else { return DataOperations.OVERWRITE; }

aokolnychyi

Looks good to me. I have been meaning to do this for quite some time. Thanks, @nastra!
We should make sure RewriteFiles stays as replace.

spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestDelete.java

amogh-jahagirdar · 2024-04-15T21:23:59Z

core/src/main/java/org/apache/iceberg/BaseRowDelta.java


  @Override
  protected String operation() {
+    if (!addsDataFiles() && addsDeleteFiles()) {


Cool, thanks for explaining! This issue is ultimately orthogonal to the one that is being solved in this PR so we can take that separately.

nastra · 2024-04-16T07:56:10Z

thanks for the reviews @amogh-jahagirdar and @aokolnychyi

nastra marked this pull request as draft April 11, 2024 14:27

github-actions bot added spark core labels Apr 11, 2024

nastra force-pushed the use-delete-op-with-delete-files branch from 2209f87 to c5dcc3a Compare April 11, 2024 14:40

amogh-jahagirdar reviewed Apr 11, 2024

View reviewed changes

nastra force-pushed the use-delete-op-with-delete-files branch 3 times, most recently from 3a32e54 to 5df00aa Compare April 12, 2024 10:06

nastra marked this pull request as ready for review April 12, 2024 10:11

nastra force-pushed the use-delete-op-with-delete-files branch from 5df00aa to bbc2212 Compare April 12, 2024 10:12

nastra commented Apr 12, 2024

View reviewed changes

nastra requested a review from amogh-jahagirdar April 12, 2024 10:33

nastra force-pushed the use-delete-op-with-delete-files branch 4 times, most recently from 6ef3220 to b0c8b28 Compare April 12, 2024 14:07

nastra commented Apr 12, 2024

View reviewed changes

nastra force-pushed the use-delete-op-with-delete-files branch 4 times, most recently from a191e0e to 29f6dd8 Compare April 13, 2024 06:43

nastra commented Apr 15, 2024

View reviewed changes

aokolnychyi reviewed Apr 15, 2024

View reviewed changes

aokolnychyi approved these changes Apr 15, 2024

View reviewed changes

aokolnychyi reviewed Apr 15, 2024

View reviewed changes

spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestDelete.java Outdated Show resolved Hide resolved

amogh-jahagirdar approved these changes Apr 15, 2024

View reviewed changes

Core, Spark: Use 'delete' if RowDelta only has delete files

0bbddda

nastra force-pushed the use-delete-op-with-delete-files branch from 29f6dd8 to 1725387 Compare April 16, 2024 06:53

review feedback

7e0237d

nastra force-pushed the use-delete-op-with-delete-files branch from 1725387 to 7e0237d Compare April 16, 2024 06:54

nastra merged commit fc5b2b3 into apache:main Apr 16, 2024

nastra deleted the use-delete-op-with-delete-files branch April 16, 2024 07:56

Fokko mentioned this pull request May 27, 2024

Support partial deletes apache/iceberg-python#569

Merged

sasankpagolu pushed a commit to sasankpagolu/iceberg that referenced this pull request Oct 27, 2024

Core: Use 'delete' if RowDelta only has delete files (apache#10123)

6d61e60

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Core: Use 'delete' if RowDelta only has delete files (apache#10123)

7752ab1

jadams-r7 mentioned this pull request Feb 12, 2025

USD-2054 Upgrade Iceberg to 1.6.1 + MySQL TX Consistency rapid7/iceberg-kafka-connect#8

Merged

pvary mentioned this pull request Nov 13, 2025

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

Merged


		Snapshot currentSnapshot = table.currentSnapshot();

		if (Boolean.parseBoolean(spark.conf().get(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key()))) {

Conversation

nastra commented Apr 11, 2024 • edited by Fokko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amogh-jahagirdar Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nastra Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nastra commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nastra commented Apr 11, 2024 •

edited by Fokko

Loading

amogh-jahagirdar Apr 11, 2024 •

edited

Loading

amogh-jahagirdar Apr 12, 2024 •

edited

Loading

amogh-jahagirdar Apr 15, 2024 •

edited

Loading

amogh-jahagirdar Apr 15, 2024 •

edited

Loading

aokolnychyi Apr 15, 2024 •

edited

Loading

nastra Apr 12, 2024 •

edited

Loading

aokolnychyi Apr 15, 2024 •

edited

Loading