kbendick commented on a change in pull request #2966:
URL: https://github.com/apache/iceberg/pull/2966#discussion_r688342383
##########
File path:
spark3/src/main/java/org/apache/iceberg/spark/actions/BaseSnapshotTableSparkAction.java
##########
@@ -168,6 +168,7 @@ public SnapshotTable tableProperty(String property, String
value) {
properties.remove(LOCATION);
properties.remove(TableProperties.WRITE_METADATA_LOCATION);
properties.remove(TableProperties.WRITE_NEW_DATA_LOCATION);
+ properties.remove(TableProperties.OBJECT_STORE_PATH);
Review comment:
Non-blocking: it might make sense to add a comment here that we’re
explicitly choosing not to bring along `OBJECT_STORE_PATH` in the snapshot?
Either a comment, or possibly updating the ObjectStorageLocationProvider
docs / snapshot docs with this detail would be great 🙂. Documentation updates
can be done in a separate PR of course (and happy to assist there if you’d
like).
##########
File path: spark3/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java
##########
@@ -225,4 +228,45 @@ public void testSetTableProperties() {
UnsupportedOperationException.class,
() -> sql("ALTER TABLE %s SET TBLPROPERTIES ('sort-order'='value')",
tableName));
}
+
+ @Test
+ public void testUpdateDataStoragePath() {
+ String objectStoragePath = "/folder/storage/path";
+ sql("ALTER TABLE %s SET TBLPROPERTIES ('%s'='true', '%s'='%s')",
+ tableName, TableProperties.OBJECT_STORE_ENABLED,
TableProperties.OBJECT_STORE_PATH, objectStoragePath);
+
+ Table table = validationCatalog.loadTable(tableIdent);
+ LocationProvider locationProvider = table.locationProvider();
+ Assert.assertEquals("should use object storage location provider",
+ "org.apache.iceberg.LocationProviders$ObjectStoreLocationProvider",
+ locationProvider.getClass().getName());
+ Assert.assertTrue("should use table object storage path",
+ locationProvider.newDataLocation("file").contains(objectStoragePath));
+
+ String folderStoragePath = "/folder/storage/path";
+ sql("ALTER TABLE %s UNSET TBLPROPERTIES ('%s')",
+ tableName, TableProperties.OBJECT_STORE_PATH);
+ sql("ALTER TABLE %s SET TBLPROPERTIES ('%s'='%s')",
+ tableName, TableProperties.WRITE_NEW_DATA_LOCATION, folderStoragePath);
+
+ table.refresh();
+ locationProvider = table.locationProvider();
+ Assert.assertEquals("should use object storage location provider",
+ "org.apache.iceberg.LocationProviders$ObjectStoreLocationProvider",
+ locationProvider.getClass().getName());
+ Assert.assertTrue("should use table folder storage path",
Review comment:
Nit: we might want to further clarify what we’re testing for in the
assertion.
Something like `Should use table folder storage path after unsetting the
object storage location path` or `should use table folder storage path if
present when object storage path is not present”.
One could argue that these assertions could be subject to the same problems
as comment rot if tests get changed, so I’ll defer to your judgement.
Also: Given that the names of the constants and their string representations
are a little funky (particularly folder storage path /
WRITE_NEW_DATA_LOCATION), it might make sense to refer to both at some point?
Again, will leave that to your discretion but I think it might help clarify for
readers. 🙂
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
