Spark: Change Delete granularity to file for Spark 3.5 #11478

amogh-jahagirdar · 2024-11-05T18:16:55Z

This changes the default delete granularity for spark 3.5 writes to file scoped. Since synchronous maintenance from #11273 was merged, we should now be able to get the best of both targeted reads and less files on disk. This makes the file scoped deletes more compelling as a default.

jbonofre · 2024-11-06T06:54:00Z

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

@@ -90,6 +90,8 @@ private static Map<String, String> persistedProperties(Map<String, String> rawPr
    persistedProperties.put(
        TableProperties.PARQUET_COMPRESSION,
        TableProperties.PARQUET_COMPRESSION_DEFAULT_SINCE_1_4_0);
+    persistedProperties.put(


By adding this, we need to update a few tests (like Hive), as we see now "write.delete.granularity"="file" in the metadata persistence.

I propose to update the tests.

Maybe worth to add a test to check how to read "previous" files which don't contain the property ?

Yes, we'll definitely need to update the failing Hive tests in their current form since it looks like they currently have expectations on the persisted properties.

Maybe worth to add a test to check how to read "previous" files which don't contain the property ?

In terms of general metadata file reading though, the core library will just read the metadata file with or without the property (since the properties is just a string-string map). In terms of how engine integrations handle the missing property, at the moment only Spark really leverages this property with a default option in case it's not defined, and we actually already have a test for verifying this default handling in case it's missing.

I'll double check the other integrations though

can we add a simple test to TestTableMetadata to make sure this is properly set on new tables and not set on existing ones?

amogh-jahagirdar

There are tests in the Spark integration that needed to be updated since they had expectations on number of output files which are now different. My general approach was to just set partition scoped deletes in the table setup if there's co verage of the file scoped case for that test class or if a given test is only useful in the context of partition scoped deletes. For rewrite procedure/action, the action tests already covers both file and partition scoped cases. The procedure also has expectations on number of files, but in the setup for the procedure test I setup partition scoped deletes since the underlying action already tests both. This minimizes how many tests need to be unnecessarily rewritten with new table layouts or expectations on output files.

amogh-jahagirdar · 2024-11-11T17:33:32Z

...ark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java

+        TableProperties.DELETE_GRANULARITY,
+        DeleteGranularity.PARTITION.toString());


This is just the default setup for a table in this test, we do test file scoped deletes as well for this action in testFileGranularity in this test class.

amogh-jahagirdar · 2024-11-11T17:36:37Z

...c/test/java/org/apache/iceberg/spark/extensions/TestRewritePositionDeleteFilesProcedure.java

@@ -49,7 +49,7 @@ private void createTable(boolean partitioned) throws Exception {
    String partitionStmt = partitioned ? "PARTITIONED BY (id)" : "";
    sql(
        "CREATE TABLE %s (id bigint, data string) USING iceberg %s TBLPROPERTIES"
-            + "('format-version'='2', 'write.delete.mode'='merge-on-read')",
+            + "('format-version'='2', 'write.delete.mode'='merge-on-read', 'write.delete.granularity'='partitioned')",


See below, the tests for the underlying action already test both partition + file granularity so we have coverage of both cases. I don't know how much value it would add to rewrite all the tests here which have expectations based on partition granularity to have expectations based on file granularity or have procedure level tests for both.

amogh-jahagirdar · 2024-11-11T17:55:11Z

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

@@ -154,7 +155,7 @@ public void testDeleteWithVectorizedReads() throws NoSuchTableException {
  }

  @Test
-  public void testCoalesceDelete() throws Exception {
+  public void testCoalesceDeleteWithPartitionGranularity() throws Exception {


I updated this test to use partition scoped deletes since I think the original intent of the test was to verify the output files when the shuffle blocks are small and coalesced into a single task (with an unpartitioned table) as part of AQE.

Without making this test specific to partition scoped deletes, we wouldn't really be testing how AQE coalescing to a single task is affecting the number of output files, which would be undifferentiated from the other tests where we already cover file scoped deletes.

This makes sense to me.

I missed that this is shared between CoW and MoR, so renaming the test name to include granularity doesn't make sense as it is here. Let me see how to fix this

manuzhang · 2024-11-13T03:18:15Z

@amogh-jahagirdar I don't see #11273 being back-ported to Spark 3.3, 3.4 yet. Shall we skip changes to Spark 3.3, 3.4 until that is done?

Meanwhile, do we have some benchmark numbers?

aokolnychyi · 2024-11-13T15:47:20Z

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

@@ -90,6 +90,8 @@ private static Map<String, String> persistedProperties(Map<String, String> rawPr
    persistedProperties.put(
        TableProperties.PARQUET_COMPRESSION,
        TableProperties.PARQUET_COMPRESSION_DEFAULT_SINCE_1_4_0);
+    persistedProperties.put(
+        TableProperties.DELETE_GRANULARITY, TableProperties.DELETE_GRANULARITY_DEFAULT_SINCE_1_8_0);


This matches what we did for zstd in Parquet. I think it should be fine as is.

An alternative is to change the default in our Spark conf classes, but it will be hard to differentiate between new and existing tables. Switching to file-scoped deletes for existing workloads may be a bigger disruption.

amogh-jahagirdar · 2024-11-13T15:56:39Z

@amogh-jahagirdar I don't see #11273 being back-ported to Spark 3.3, 3.4 yet. Shall we skip changes to Spark 3.3, 3.4 until that is done?

@manuzhang Definitely agree that we shouldn't merge this change to 3.3/3.4 until the maintenance piece is backported, I'm working on the backport. Though I think we can leave this PR open for discussion, and I also wanted to raise this to see which tests would need to be updated from CI results (and I wanted to get some clarity if 3.3/3.4 tests were different somehow or not).

What I'd propose is we backport the sync maintenance to 3.3/3.4 since I think we'll need that anyways, and then if there's consensus here/mailing list to change the default table property then we could just update all the spark tests in one go.
Alternatively like @aokolnychyi mentioned above we could just change the specific spark conf in the case we don't want to backport to older spark versions. I am leaning towards just doing the backport because I think we should just take a stance if we think the new default is just going to be generally better, rather than making it specific to a spark version.

nastra · 2024-11-13T15:59:42Z

I'm in favor of doing a normal backport so that all Spark versions get the same improvement

amogh-jahagirdar · 2024-12-12T16:12:37Z

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

@@ -231,7 +233,6 @@ public void testMergeWithVectorizedReads() {

  @TestTemplate
  public void testCoalesceMerge() {
-    assumeThat(formatVersion).isLessThan(3);


Addressing a miss from my previous PR for writing DVs in V3, instead of skipping this coalesce case when the format version is less than 3, we should be asserting the expected DVs (the else if case below)

aokolnychyi · 2024-12-13T22:53:19Z

spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java

@@ -719,7 +719,7 @@ public DeleteGranularity deleteGranularity() {
            .stringConf()
            .option(SparkWriteOptions.DELETE_GRANULARITY)
            .tableProperty(TableProperties.DELETE_GRANULARITY)
-            .defaultValue(TableProperties.DELETE_GRANULARITY_DEFAULT)
+            .defaultValue(DeleteGranularity.FILE.toString())


Let's switch to enumConf for this one, which was added recently.

aokolnychyi

LGTM pending the discussion on the dev list.

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

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

RussellSpitzer

Looks good to me, just a minor few nits on branches which are now redundant

amogh-jahagirdar · 2025-01-03T02:46:26Z

Unrelated test failure:

TestCopyOnWriteDelete > testDeleteWithSnapshotIsolation() > catalogName = testhive, implementation = org.apache.iceberg.spark.SparkCatalog, config = {type=hive, default-namespace=default}, format = PARQUET, vectorized = true, distributionMode = none, fanout = false, branch = test, planningMode = DISTRIBUTED, formatVersion = 2 FAILED
    java.util.concurrent.ExecutionException: org.apache.iceberg.exceptions.CommitFailedException: Cannot commit: Base metadata location 'file:/tmp/hive2727347621790535071/table/metadata/00046-e8031c63-6512-4cb1-9f2d-38848b7f1a6d.metadata.json' is not same as the current table metadata location 'file:/tmp/hive2727347621790535071/table/metadata/00047-2d3165bb-4939-4288-923d-74b30d4a1f3d.metadata.json' for default.table

There are two Iceberg PRs that "broke" NesQuEIT: * apache/iceberg#11478 caused `testRewriteManifests` to fail due to the changed outcome of the `rewrite_manifests` procedure * apache/iceberg#11520 caused a class-path issue w/ Scala 2.13

github-actions bot added the core label Nov 5, 2024

amogh-jahagirdar changed the title ~~Change delete granularity~~ Core: Change Delete granularity to file for new tables Nov 5, 2024

amogh-jahagirdar force-pushed the change-delete-granularity branch from 69a8821 to e4e1f0f Compare November 6, 2024 02:15

github-actions bot added the spark label Nov 6, 2024

amogh-jahagirdar force-pushed the change-delete-granularity branch 2 times, most recently from 95301d5 to 66f8307 Compare November 6, 2024 02:47

jbonofre suggested changes Nov 6, 2024

View reviewed changes

amogh-jahagirdar force-pushed the change-delete-granularity branch from 66f8307 to 84e133d Compare November 8, 2024 16:24

github-actions bot added the MR label Nov 8, 2024

amogh-jahagirdar force-pushed the change-delete-granularity branch 7 times, most recently from 07c0b77 to b484a7e Compare November 11, 2024 17:59

amogh-jahagirdar commented Nov 11, 2024

View reviewed changes

amogh-jahagirdar marked this pull request as ready for review November 11, 2024 18:53

amogh-jahagirdar requested review from jbonofre, nastra and aokolnychyi November 11, 2024 18:53

aokolnychyi reviewed Nov 13, 2024

View reviewed changes

amogh-jahagirdar force-pushed the change-delete-granularity branch from b484a7e to a397312 Compare December 10, 2024 16:31

github-actions bot removed the MR label Dec 10, 2024

amogh-jahagirdar force-pushed the change-delete-granularity branch 3 times, most recently from c08d307 to 137c20b Compare December 10, 2024 16:41

amogh-jahagirdar force-pushed the change-delete-granularity branch 3 times, most recently from e5332cf to d4e9ca5 Compare December 12, 2024 16:07

amogh-jahagirdar commented Dec 12, 2024

View reviewed changes

amogh-jahagirdar changed the title ~~Core: Change Delete granularity to file for new tables~~ Spark: Change Delete granularity to file for Spark 3.5 Dec 12, 2024

amogh-jahagirdar requested a review from aokolnychyi December 12, 2024 16:14

amogh-jahagirdar force-pushed the change-delete-granularity branch from d4e9ca5 to b5ac881 Compare December 13, 2024 19:39

aokolnychyi reviewed Dec 13, 2024

View reviewed changes

aokolnychyi approved these changes Dec 13, 2024

View reviewed changes

nastra approved these changes Dec 16, 2024

View reviewed changes

RussellSpitzer reviewed Jan 2, 2025

View reviewed changes

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

RussellSpitzer reviewed Jan 2, 2025

View reviewed changes

spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestUpdate.java Show resolved Hide resolved

RussellSpitzer approved these changes Jan 2, 2025

View reviewed changes

Spark: Change delete file granularity to file in Spark 3.5

17c1a0a

amogh-jahagirdar force-pushed the change-delete-granularity branch from b5ac881 to 17c1a0a Compare January 2, 2025 21:01

amogh-jahagirdar merged commit c0d6d42 into apache:main Jan 3, 2025
31 checks passed

snazy mentioned this pull request Jan 10, 2025

Fix NesQuEIT to pass against recent Iceberg changes projectnessie/nessie#10184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spark: Change Delete granularity to file for Spark 3.5 #11478

Spark: Change Delete granularity to file for Spark 3.5 #11478

amogh-jahagirdar commented Nov 5, 2024 •

edited

Loading

jbonofre Nov 6, 2024

amogh-jahagirdar Nov 8, 2024

nastra Nov 12, 2024

amogh-jahagirdar left a comment

amogh-jahagirdar Nov 11, 2024

amogh-jahagirdar Nov 11, 2024

amogh-jahagirdar Nov 11, 2024

aokolnychyi Nov 13, 2024

amogh-jahagirdar Nov 19, 2024 •

edited

Loading

manuzhang commented Nov 13, 2024 •

edited

Loading

aokolnychyi Nov 13, 2024 •

edited

Loading

amogh-jahagirdar commented Nov 13, 2024 •

edited

Loading

nastra commented Nov 13, 2024

amogh-jahagirdar Dec 12, 2024

aokolnychyi Dec 13, 2024

aokolnychyi left a comment

RussellSpitzer left a comment

amogh-jahagirdar commented Jan 3, 2025

		TableProperties.DELETE_GRANULARITY,
		DeleteGranularity.PARTITION.toString());

Spark: Change Delete granularity to file for Spark 3.5 #11478

Spark: Change Delete granularity to file for Spark 3.5 #11478

Conversation

amogh-jahagirdar commented Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

manuzhang commented Nov 13, 2024 • edited Loading

aokolnychyi Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

amogh-jahagirdar commented Nov 13, 2024 • edited Loading

nastra commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi left a comment

Choose a reason for hiding this comment

RussellSpitzer left a comment

Choose a reason for hiding this comment

amogh-jahagirdar commented Jan 3, 2025

amogh-jahagirdar commented Nov 5, 2024 •

edited

Loading

amogh-jahagirdar Nov 19, 2024 •

edited

Loading

manuzhang commented Nov 13, 2024 •

edited

Loading

aokolnychyi Nov 13, 2024 •

edited

Loading

amogh-jahagirdar commented Nov 13, 2024 •

edited

Loading