Spark: Add location overlap validation for SnapshotTableAction #14933

varun-lakhyani · 2025-12-27T09:08:56Z

Description

Implements location overlap validation for SnapshotTableAction to prevent destination table location from overlapping with source table location.

Resolves TODO comment in SnapshotTableSparkAction.java:127 in spark v4.1.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Added unit tests covering snapshot and source location overlap scenarios:
- Exact same location
- Source as a subdirectory of destination
- Destination as a subdirectory of source
- Completely non-overlapping locations
All CI checks pass (31/31)

Checklist

Implementation follows existing code patterns and Iceberg contribution guidelines
Code compiles without errors
Test coverage added
No breaking changes to public APIs

…ng destination table location from overlaping source table location. Resolves TODO comment in SnapshotTableSparkAction.java for spark v4.1

RussellSpitzer · 2025-12-28T02:59:01Z

Definitely needs a test case, not sure why windows would make testing impossible since our locations are stored as strings

RussellSpitzer · 2025-12-28T03:01:24Z

Iceberg errors follow this format

"Cannot X because Y (possibly fix Z)"

So in this case

Cannot create a snapshot at location ... because it would overlap with source table ... Overlapping snapshot and source would ....

varun-lakhyani · 2025-12-28T21:55:58Z

@RussellSpitzer

Iceberg errors follow this format

"Cannot X because Y (possibly fix Z)"

So in this case

Cannot create a snapshot at location ... because it would overlap with source table ... Overlapping snapshot and source would ....

Thanks for the feedback.
Added unit tests covering overlapping snapshot and source locations (same path, parent/child cases, and non-overlapping paths) and have updated the error message to follow the Iceberg format.

All CI checks are passing now.

varun-lakhyani · 2025-12-31T09:31:19Z

Iceberg errors follow this format

"Cannot X because Y (possibly fix Z)"

So in this case

Cannot create a snapshot at location ... because it would overlap with source table ... Overlapping snapshot and source would ....

@RussellSpitzer
I have addressed all your feedback - added tests and updated error formatting.
All CI checks passing. Would appreciate a review when you are available, Happy to make any changes if required.
Thanks

varun-lakhyani · 2026-01-07T09:42:20Z

@RussellSpitzer PTAL

varun-lakhyani · 2026-01-09T04:29:26Z

PTAL @pvary

pvary · 2026-01-09T10:38:58Z

spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/SnapshotTableSparkAction.java


-    // TODO: Check the dest table location does not overlap with the source table location
+    String sourceTableLocation = sourceTableLocation();
+    String actualDestTableLocation = (icebergTable.location());


maybe stagedTableLocation?
Also, please remove the extra parentheses.

Definitely Looks more appropriate, Will Commit with other changes.

pvary · 2026-01-09T10:40:51Z

spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java

+  @TestTemplate
+  public void testSnapshotWithOverlappingLocation() throws IOException {
+    String catalogType = catalogConfig.get(ICEBERG_CATALOG_TYPE);
+    assumeThat(catalogType).isNotEqualTo(ICEBERG_CATALOG_TYPE_HADOOP);


Why do we don't test with hadoop?

Why do we don't test with hadoop?

Hadoop doesn't allow custom destination location via .tableLocation() when done this it fails even before the check while StagedSparkTable stagedTable = stageDestTable();

Error Message:
TestSnapshotTableAction > testSnapshotWithOverlappingLocation() > catalogName = testhadoop, implementation = org.apache.iceberg.spark.SparkCatalog, config = {type=hadoop, cache-enabled=false} FAILED java.lang.IllegalArgumentException: Cannot set a custom location for a path-based table. Expected file:/tmp/warehouse62334905503302693.tmp/default/table but got /tmp/junit-17415081344650343329/newJunit10343828823716601592

because of HadoopCatalog withLocation() check

public TableBuilder withLocation(String location) { Preconditions.checkArgument( location == null || location.equals(defaultLocation), "Cannot set a custom location for a path-based table. Expected " + defaultLocation + " but got " + location); return this; }

So, when we can't give custom location and TestBase predefine the warehouse using which hadoop always derives non-overlapping location the test seems useless unless we change the warehouse.
For the same reason I skipped Hadoop catalog for the tests including .tableLocation().
Do let me know If I should think otherwise

Best to just add a note here, "//Hadoop Catalogs do not Support Custom Table Locations"

pvary · 2026-01-09T10:50:22Z

spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java

+    String validDestLocation = new Path(parentLocation, "newDestination").toUri().toString();
+    SparkActions.get()
+        .snapshotTable(SOURCE_NAME)
+        .as(tableName)
+        .tableLocation(validDestLocation)
+        .execute();
+    assertThat(sql("SELECT * FROM %s", tableName)).hasSize(2);


Do we need this test here?
Should this be a different test case for the happy path?

Along with Overlapping location tests, A Non Overlapping test should also be tested.

Yes, This test should not be in TestSnapshotWithOverlappingLocation so I have separated TestSnapshotWithNonOverlappingLocation and placed this assertion in it

varun-lakhyani · 2026-01-09T13:49:20Z

@pvary I have addressed the reviews. Please take another look and let me know if any concerns

RussellSpitzer · 2026-01-07T16:19:20Z

spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java

 import java.nio.file.Files;
 import java.util.concurrent.Executors;
 import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.fs.Path;


Not sure we need Hadoop's Path class here, let's stick to Java built ins if we can

RussellSpitzer

My only remaining concern here is we are using the Hadoop Path class and I"m pretty sure we don't need that dependency here

varun-lakhyani added 2 commits December 27, 2025 14:26

Add Location overlap validation for SnapshotTableSparkAction preventi…

bf0ee55

…ng destination table location from overlaping source table location. Resolves TODO comment in SnapshotTableSparkAction.java for spark v4.1

Add Location overlap validation for SnapshotTableSparkAction preventi…

68c1a8c

…ng destination table location from overlaping source table location. Resolves TODO comment in SnapshotTableSparkAction.java for spark v4.1

github-actions bot added the spark label Dec 27, 2025

Add Location overlap validation for SnapshotTableSparkAction preventi…

5c54330

…ng destination table location from overlaping source table location. Resolves TODO comment in SnapshotTableSparkAction.java for spark v4.1

varun-lakhyani added 6 commits December 28, 2025 09:18

Fix snapshot location overlap error message

5671a8a

Fix snapshot location overlap error message

712c9bd

Add test for overlapping snapshot and source locations

3beb3b1

Validate snapshot destination and source overlap prior to staging

1a3dcf0

Fix snapshot location path construction in overlap tests

dae2084

Fix snapshot location path construction in overlap tests

fb9e76d

pvary reviewed Jan 9, 2026

View reviewed changes

Rename variables and separate non-overlapping tests

a365b1f

RussellSpitzer reviewed Jan 9, 2026

View reviewed changes

RussellSpitzer requested changes Jan 9, 2026

View reviewed changes

Spark: Add location overlap validation for SnapshotTableAction #14933

Are you sure you want to change the base?

Spark: Add location overlap validation for SnapshotTableAction #14933

Conversation

varun-lakhyani commented Dec 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist

Uh oh!

RussellSpitzer commented Dec 28, 2025

Uh oh!

RussellSpitzer commented Dec 28, 2025

Uh oh!

varun-lakhyani commented Dec 28, 2025

Uh oh!

varun-lakhyani commented Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varun-lakhyani commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varun-lakhyani commented Jan 9, 2026

Uh oh!

pvary Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

varun-lakhyani Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

pvary Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

varun-lakhyani Jan 9, 2026 • 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!

RussellSpitzer Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

pvary Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

varun-lakhyani Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

varun-lakhyani commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RussellSpitzer Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer left a comment • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

varun-lakhyani commented Dec 27, 2025 •

edited

Loading

varun-lakhyani commented Dec 31, 2025 •

edited

Loading

varun-lakhyani commented Jan 7, 2026 •

edited

Loading

varun-lakhyani Jan 9, 2026 •

edited

Loading

varun-lakhyani commented Jan 9, 2026 •

edited

Loading

RussellSpitzer left a comment •

edited

Loading