Skip to content

Spark: Add location overlap validation for SnapshotTableAction #14933

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 11 commits into from
Jan 10, 2026
Merged

Spark: Add location overlap validation for SnapshotTableAction #14933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bf0ee55
Add Location overlap validation for SnapshotTableSparkAction preventi…
varun-lakhyani Dec 27, 2025
68c1a8c
Add Location overlap validation for SnapshotTableSparkAction preventi…
varun-lakhyani Dec 27, 2025
5c54330
Add Location overlap validation for SnapshotTableSparkAction preventi…
varun-lakhyani Dec 27, 2025
5671a8a
Fix snapshot location overlap error message
varun-lakhyani Dec 28, 2025
712c9bd
Fix snapshot location overlap error message
varun-lakhyani Dec 28, 2025
3beb3b1
Add test for overlapping snapshot and source locations
varun-lakhyani Dec 28, 2025
1a3dcf0
Validate snapshot destination and source overlap prior to staging
varun-lakhyani Dec 28, 2025
dae2084
Fix snapshot location path construction in overlap tests
varun-lakhyani Dec 28, 2025
fb9e76d
Fix snapshot location path construction in overlap tests
varun-lakhyani Dec 28, 2025
a365b1f
Rename variables and separate non-overlapping tests
varun-lakhyani Jan 9, 2026
de3107b
Remove Hadoop Path dependency
varun-lakhyani Jan 9, 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
11 changes: 10 additions & 1 deletion ...k/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/SnapshotTableSparkAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,16 @@ private SnapshotTable.Result doExecute() {
StagedSparkTable stagedTable = stageDestTable();
Table icebergTable = stagedTable.table();

// TODO: Check the dest table location does not overlap with the source table location
String sourceTableLocation = sourceTableLocation();
String stagedTableLocation = icebergTable.location();
Preconditions.checkArgument(
!sourceTableLocation.equals(stagedTableLocation)
&& !stagedTableLocation.startsWith(sourceTableLocation + "/")
&& !sourceTableLocation.startsWith(stagedTableLocation + "/"),
"Cannot create a snapshot at location %s because it would overlap with source table location %s. "
+ "Overlapping snapshot and source would mix table files.",
stagedTableLocation,
sourceTableLocation);

boolean threw = true;
try {
Expand Down
93 changes: 93 additions & 0 deletions spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@
*/
package org.apache.iceberg.spark.actions;

import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE;
import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_HADOOP;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.Assumptions.assumeThat;

import java.io.IOException;
import java.nio.file.Files;
Expand All @@ -33,6 +37,7 @@
@ExtendWith(ParameterizedTestExtension.class)
public class TestSnapshotTableAction extends CatalogTestBase {
private static final String SOURCE_NAME = "spark_catalog.default.source";
private static final String SOURCE = "source";

@AfterEach
public void removeTables() {
Expand Down Expand Up @@ -65,4 +70,92 @@ public void testSnapshotWithParallelTasks() throws IOException {
.execute();
assertThat(snapshotThreadsIndex.get()).isEqualTo(2);
}

@TestTemplate
public void testSnapshotWithOverlappingLocation() throws IOException {
// Hadoop Catalogs do not Support Custom Table Locations
String catalogType = catalogConfig.get(ICEBERG_CATALOG_TYPE);
assumeThat(catalogType).isNotEqualTo(ICEBERG_CATALOG_TYPE_HADOOP);
Copy link
Contributor

@pvary pvary Jan 9, 2026

Choose a reason for hiding this comment

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

Why do we don't test with hadoop?

Copy link
Contributor Author

@varun-lakhyani varun-lakhyani Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@RussellSpitzer RussellSpitzer Jan 9, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@varun-lakhyani varun-lakhyani Jan 9, 2026

Choose a reason for hiding this comment

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

Updated in latest commit


String sourceLocation =
Files.createTempDirectory(temp, "junit").resolve(SOURCE).toFile().toString();
sql(
"CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
SOURCE_NAME, sourceLocation);
sql("INSERT INTO TABLE %s VALUES (1, 'a')", SOURCE_NAME);
sql("INSERT INTO TABLE %s VALUES (2, 'b')", SOURCE_NAME);
String actualSourceLocation =
spark
.sql(String.format("DESCRIBE EXTENDED %s", SOURCE_NAME))
.filter("col_name = 'Location'")
.select("data_type")
.first()
.getString(0);

assertThatThrownBy(
() ->
SparkActions.get()
.snapshotTable(SOURCE_NAME)
.as(tableName)
.tableLocation(actualSourceLocation)
.execute())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith(
"The snapshot table location cannot be same as the source table location.");

String destAsSubdirectory = actualSourceLocation + "/nested";
assertThatThrownBy(
() ->
SparkActions.get()
.snapshotTable(SOURCE_NAME)
.as(tableName)
.tableLocation(destAsSubdirectory)
.execute())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("Cannot create a snapshot at location");

String parentLocation =
actualSourceLocation.substring(0, actualSourceLocation.length() - ("/" + SOURCE).length());
assertThatThrownBy(
() ->
SparkActions.get()
.snapshotTable(SOURCE_NAME)
.as(tableName)
.tableLocation(parentLocation)
.execute())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("Cannot create a snapshot at location");
}

@TestTemplate
public void testSnapshotWithNonOverlappingLocation() throws IOException {
// Hadoop Catalogs do not Support Custom Table Locations
String catalogType = catalogConfig.get(ICEBERG_CATALOG_TYPE);
assumeThat(catalogType).isNotEqualTo(ICEBERG_CATALOG_TYPE_HADOOP);

String sourceLocation =
Files.createTempDirectory(temp, "junit").resolve(SOURCE).toFile().toString();
sql(
"CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
SOURCE_NAME, sourceLocation);
sql("INSERT INTO TABLE %s VALUES (1, 'a')", SOURCE_NAME);
sql("INSERT INTO TABLE %s VALUES (2, 'b')", SOURCE_NAME);
String actualSourceLocation =
spark
.sql(String.format("DESCRIBE EXTENDED %s", SOURCE_NAME))
.filter("col_name = 'Location'")
.select("data_type")
.first()
.getString(0);

String validDestLocation =
actualSourceLocation.substring(0, actualSourceLocation.length() - SOURCE.length())
+ "newDestination";
SparkActions.get()
.snapshotTable(SOURCE_NAME)
.as(tableName)
.tableLocation(validDestLocation)
.execute();
assertThat(sql("SELECT * FROM %s", tableName)).hasSize(2);
}
}