You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "varun-lakhyani (via GitHub)" <gi...@apache.org> on 2025/12/27 09:08:59 UTC
[PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
varun-lakhyani opened a new pull request, #14933:
URL: https://github.com/apache/iceberg/pull/14933
## 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
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation update
## How Has This Been Tested?
- Code compiles successfully with Java 17
- Local testing skipped due to Windows POSIX permissions limitation
## Checklist
- [x] Implementation follows existing code patterns and Iceberg contribution guidelines
- [x] Code compiles without errors
- [ ] Test coverage added (will add if requested during review)
- [x] No breaking changes to public APIs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2677357476
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -18,12 +18,17 @@
*/
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;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.fs.Path;
Review Comment:
Yes Correct, It was just used for creating nested path locations.
Standard string operations are simpler and suitable than this. UPDATED in latest commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#issuecomment-3694402470
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 ....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#issuecomment-3731896555
Merged to main.
Thanks @varun-lakhyani for implementing the `TODO`, and @RussellSpitzer for the review.
@varun-lakhyani: Please don't forget to create a backport PR for the other Spark versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2675880601
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -65,4 +70,66 @@ public void testSnapshotWithParallelTasks() throws IOException {
.execute();
assertThat(snapshotThreadsIndex.get()).isEqualTo(2);
}
+
+ @TestTemplate
+ public void testSnapshotWithOverlappingLocation() throws IOException {
+ String catalogType = catalogConfig.get(ICEBERG_CATALOG_TYPE);
+ assumeThat(catalogType).isNotEqualTo(ICEBERG_CATALOG_TYPE_HADOOP);
Review Comment:
> 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() and didn't create another specially for it.
Do let me know If I should think otherwise
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2669111016
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -18,12 +18,17 @@
*/
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;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.fs.Path;
Review Comment:
Not sure we need Hadoop's Path class here, let's stick to Java built ins if we can
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#issuecomment-3727146551
PTAL @pvary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2675688499
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/SnapshotTableSparkAction.java:
##########
@@ -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 actualDestTableLocation = (icebergTable.location());
Review Comment:
maybe `stagedTableLocation`?
Also, please remove the extra parentheses.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2676253096
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -65,4 +70,66 @@ public void testSnapshotWithParallelTasks() throws IOException {
.execute();
assertThat(snapshotThreadsIndex.get()).isEqualTo(2);
}
+
+ @TestTemplate
+ public void testSnapshotWithOverlappingLocation() throws IOException {
+ 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);
+
+ 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 = new Path(actualSourceLocation, "nested").toUri().toString();
+ assertThatThrownBy(
+ () ->
+ SparkActions.get()
+ .snapshotTable(SOURCE_NAME)
+ .as(tableName)
+ .tableLocation(destAsSubdirectory)
+ .execute())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageStartingWith("Cannot create a snapshot at location");
+
+ String parentLocation = new Path(actualSourceLocation).getParent().toUri().toString();
+ assertThatThrownBy(
+ () ->
+ SparkActions.get()
+ .snapshotTable(SOURCE_NAME)
+ .as(tableName)
+ .tableLocation(parentLocation)
+ .execute())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageStartingWith("Cannot create a snapshot at location");
+
+ 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);
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#issuecomment-3695078026
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2675718794
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -65,4 +70,66 @@ public void testSnapshotWithParallelTasks() throws IOException {
.execute();
assertThat(snapshotThreadsIndex.get()).isEqualTo(2);
}
+
+ @TestTemplate
+ public void testSnapshotWithOverlappingLocation() throws IOException {
+ String catalogType = catalogConfig.get(ICEBERG_CATALOG_TYPE);
+ assumeThat(catalogType).isNotEqualTo(ICEBERG_CATALOG_TYPE_HADOOP);
+
+ String sourceLocation =
+ Files.createTempDirectory(temp, "junit").resolve("source").toFile().toString();
Review Comment:
Why do we create an extra `source` directory here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2675718794
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -65,4 +70,66 @@ public void testSnapshotWithParallelTasks() throws IOException {
.execute();
assertThat(snapshotThreadsIndex.get()).isEqualTo(2);
}
+
+ @TestTemplate
+ public void testSnapshotWithOverlappingLocation() throws IOException {
+ String catalogType = catalogConfig.get(ICEBERG_CATALOG_TYPE);
+ assumeThat(catalogType).isNotEqualTo(ICEBERG_CATALOG_TYPE_HADOOP);
+
+ String sourceLocation =
+ Files.createTempDirectory(temp, "junit").resolve("source").toFile().toString();
Review Comment:
Why do we create an extra `source` directory here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2675724549
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -65,4 +70,66 @@ public void testSnapshotWithParallelTasks() throws IOException {
.execute();
assertThat(snapshotThreadsIndex.get()).isEqualTo(2);
}
+
+ @TestTemplate
+ public void testSnapshotWithOverlappingLocation() throws IOException {
+ 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);
+
+ 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 = new Path(actualSourceLocation, "nested").toUri().toString();
+ assertThatThrownBy(
+ () ->
+ SparkActions.get()
+ .snapshotTable(SOURCE_NAME)
+ .as(tableName)
+ .tableLocation(destAsSubdirectory)
+ .execute())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageStartingWith("Cannot create a snapshot at location");
+
+ String parentLocation = new Path(actualSourceLocation).getParent().toUri().toString();
+ assertThatThrownBy(
+ () ->
+ SparkActions.get()
+ .snapshotTable(SOURCE_NAME)
+ .as(tableName)
+ .tableLocation(parentLocation)
+ .execute())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageStartingWith("Cannot create a snapshot at location");
+
+ 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);
Review Comment:
Do we need this test here?
Should this be a different test case for the happy path?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2675880601
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -65,4 +70,66 @@ public void testSnapshotWithParallelTasks() throws IOException {
.execute();
assertThat(snapshotThreadsIndex.get()).isEqualTo(2);
}
+
+ @TestTemplate
+ public void testSnapshotWithOverlappingLocation() throws IOException {
+ String catalogType = catalogConfig.get(ICEBERG_CATALOG_TYPE);
+ assumeThat(catalogType).isNotEqualTo(ICEBERG_CATALOG_TYPE_HADOOP);
Review Comment:
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#issuecomment-3728987930
@pvary I have addressed the reviews. Please take another look and let me know any concerns
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2676702244
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -65,4 +70,66 @@ public void testSnapshotWithParallelTasks() throws IOException {
.execute();
assertThat(snapshotThreadsIndex.get()).isEqualTo(2);
}
+
+ @TestTemplate
+ public void testSnapshotWithOverlappingLocation() throws IOException {
+ String catalogType = catalogConfig.get(ICEBERG_CATALOG_TYPE);
+ assumeThat(catalogType).isNotEqualTo(ICEBERG_CATALOG_TYPE_HADOOP);
Review Comment:
Best to just add a note here, "//Hadoop Catalogs do not Support Custom Table Locations"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#issuecomment-3694401289
Definitely needs a test case, not sure why windows would make testing impossible since our locations are stored as strings
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #14933:
URL: https://github.com/apache/iceberg/pull/14933
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2675907473
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/SnapshotTableSparkAction.java:
##########
@@ -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 actualDestTableLocation = (icebergTable.location());
Review Comment:
Definitely Looks more appropriate, Will Commit with other changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2677353435
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -65,4 +70,66 @@ public void testSnapshotWithParallelTasks() throws IOException {
.execute();
assertThat(snapshotThreadsIndex.get()).isEqualTo(2);
}
+
+ @TestTemplate
+ public void testSnapshotWithOverlappingLocation() throws IOException {
+ String catalogType = catalogConfig.get(ICEBERG_CATALOG_TYPE);
+ assumeThat(catalogType).isNotEqualTo(ICEBERG_CATALOG_TYPE_HADOOP);
Review Comment:
Updated in latest commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#issuecomment-3718052238
@RussellSpitzer PTAL at this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#issuecomment-3733279582
Thanks @RussellSpitzer @pvary for looking into this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#issuecomment-3730271788
> 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
Removed Hadoop Path Class dependency.
Please take another look and let me know if any other changes. @RussellSpitzer @pvary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#discussion_r2675694536
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestSnapshotTableAction.java:
##########
@@ -65,4 +70,66 @@ public void testSnapshotWithParallelTasks() throws IOException {
.execute();
assertThat(snapshotThreadsIndex.get()).isEqualTo(2);
}
+
+ @TestTemplate
+ public void testSnapshotWithOverlappingLocation() throws IOException {
+ String catalogType = catalogConfig.get(ICEBERG_CATALOG_TYPE);
+ assumeThat(catalogType).isNotEqualTo(ICEBERG_CATALOG_TYPE_HADOOP);
Review Comment:
Why do we don't test with hadoop?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add location overlap validation for SnapshotTableAction [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on PR #14933:
URL: https://github.com/apache/iceberg/pull/14933#issuecomment-3701828920
> 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.
Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org