diff --git a/core/src/main/java/org/apache/iceberg/LocationProviders.java b/core/src/main/java/org/apache/iceberg/LocationProviders.java index 68bec2f4e4fc..6a75529ddb91 100644 --- a/core/src/main/java/org/apache/iceberg/LocationProviders.java +++ b/core/src/main/java/org/apache/iceberg/LocationProviders.java @@ -20,10 +20,12 @@ import java.nio.charset.StandardCharsets; import java.util.Map; +import java.util.Set; import org.apache.hadoop.fs.Path; import org.apache.iceberg.common.DynConstructors; import org.apache.iceberg.io.LocationProvider; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; import org.apache.iceberg.relocated.com.google.common.hash.HashCode; import org.apache.iceberg.relocated.com.google.common.hash.HashFunction; import org.apache.iceberg.relocated.com.google.common.hash.Hashing; @@ -75,6 +77,23 @@ public static LocationProvider locationsFor( } } + private static final Set DEPRECATED_PROPERTIES = + ImmutableSet.of( + TableProperties.OBJECT_STORE_PATH, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); + + private static String getAndCheckLegacyLocation(Map properties, String key) { + String value = properties.get(key); + + if (value != null && DEPRECATED_PROPERTIES.contains(key)) { + throw new IllegalArgumentException( + String.format( + "Property '%s' has been deprecated and will be removed in 2.0, use '%s' instead.", + key, TableProperties.WRITE_DATA_LOCATION)); + } + + return value; + } + static class DefaultLocationProvider implements LocationProvider { private final String dataLocation; @@ -83,9 +102,11 @@ static class DefaultLocationProvider implements LocationProvider { } private static String dataLocation(Map properties, String tableLocation) { - String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION); + String dataLocation = + getAndCheckLegacyLocation(properties, TableProperties.WRITE_DATA_LOCATION); if (dataLocation == null) { - dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION); + dataLocation = + getAndCheckLegacyLocation(properties, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); if (dataLocation == null) { dataLocation = String.format("%s/data", tableLocation); } @@ -135,11 +156,13 @@ static class ObjectStoreLocationProvider implements LocationProvider { } private static String dataLocation(Map properties, String tableLocation) { - String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION); + String dataLocation = + getAndCheckLegacyLocation(properties, TableProperties.WRITE_DATA_LOCATION); if (dataLocation == null) { - dataLocation = properties.get(TableProperties.OBJECT_STORE_PATH); + dataLocation = getAndCheckLegacyLocation(properties, TableProperties.OBJECT_STORE_PATH); if (dataLocation == null) { - dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION); + dataLocation = + getAndCheckLegacyLocation(properties, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); if (dataLocation == null) { dataLocation = String.format("%s/data", tableLocation); } diff --git a/core/src/main/java/org/apache/iceberg/TableProperties.java b/core/src/main/java/org/apache/iceberg/TableProperties.java index cd7cda23c2d3..b3505be3ce08 100644 --- a/core/src/main/java/org/apache/iceberg/TableProperties.java +++ b/core/src/main/java/org/apache/iceberg/TableProperties.java @@ -250,14 +250,14 @@ private TableProperties() {} public static final boolean WRITE_OBJECT_STORE_PARTITIONED_PATHS_DEFAULT = true; /** - * @deprecated Use {@link #WRITE_DATA_LOCATION} instead. + * @deprecated will be removed in 2.0.0, use {@link #WRITE_DATA_LOCATION} instead. */ @Deprecated public static final String OBJECT_STORE_PATH = "write.object-storage.path"; public static final String WRITE_LOCATION_PROVIDER_IMPL = "write.location-provider.impl"; /** - * @deprecated Use {@link #WRITE_DATA_LOCATION} instead. + * @deprecated will be removed in 2.0.0, use {@link #WRITE_DATA_LOCATION} instead. */ @Deprecated public static final String WRITE_FOLDER_STORAGE_LOCATION = "write.folder-storage.path"; diff --git a/core/src/test/java/org/apache/iceberg/TestLocationProvider.java b/core/src/test/java/org/apache/iceberg/TestLocationProvider.java index 72967115ea24..dfe4d946c331 100644 --- a/core/src/test/java/org/apache/iceberg/TestLocationProvider.java +++ b/core/src/test/java/org/apache/iceberg/TestLocationProvider.java @@ -209,61 +209,44 @@ public void testInvalidArgTypesDynamicallyLoadedLocationProvider() { } @TestTemplate - public void testObjectStorageLocationProviderPathResolution() { - table.updateProperties().set(TableProperties.OBJECT_STORE_ENABLED, "true").commit(); - - assertThat(table.locationProvider().newDataLocation("file")) - .as("default data location should be used when object storage path not set") - .contains(table.location() + "/data"); - - String folderPath = "s3://random/folder/location"; + public void testObjectStorageLocationProviderThrowOnDeprecatedProperties() { + String objectPath = "s3://random/object/location"; table .updateProperties() - .set(TableProperties.WRITE_FOLDER_STORAGE_LOCATION, folderPath) + .set(TableProperties.OBJECT_STORE_ENABLED, "true") + .set(TableProperties.WRITE_FOLDER_STORAGE_LOCATION, objectPath) .commit(); - assertThat(table.locationProvider().newDataLocation("file")) - .as("folder storage path should be used when set") - .contains(folderPath); - - String objectPath = "s3://random/object/location"; - table.updateProperties().set(TableProperties.OBJECT_STORE_PATH, objectPath).commit(); + assertThatThrownBy(() -> table.locationProvider().newDataLocation("file")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage( + "Property 'write.folder-storage.path' has been deprecated and will be removed in 2.0, use 'write.data.path' instead."); - assertThat(table.locationProvider().newDataLocation("file")) - .as("object storage path should be used when set") - .contains(objectPath); + table + .updateProperties() + .set(TableProperties.OBJECT_STORE_PATH, objectPath) + .remove(TableProperties.WRITE_FOLDER_STORAGE_LOCATION) + .commit(); - String dataPath = "s3://random/data/location"; - table.updateProperties().set(TableProperties.WRITE_DATA_LOCATION, dataPath).commit(); - assertThat(table.locationProvider().newDataLocation("file")) - .as("write data path should be used when set") - .contains(dataPath); + assertThatThrownBy(() -> table.locationProvider().newDataLocation("file")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage( + "Property 'write.object-storage.path' has been deprecated and will be removed in 2.0, use 'write.data.path' instead."); } @TestTemplate - public void testDefaultStorageLocationProviderPathResolution() { - table.updateProperties().set(TableProperties.OBJECT_STORE_ENABLED, "false").commit(); - - assertThat(table.locationProvider().newDataLocation("file")) - .as("default data location should be used when object storage path not set") - .contains(table.location() + "/data"); - + public void testDefaultStorageLocationProviderThrowOnDeprecatedProperties() { String folderPath = "s3://random/folder/location"; table .updateProperties() + .set(TableProperties.OBJECT_STORE_ENABLED, "false") .set(TableProperties.WRITE_FOLDER_STORAGE_LOCATION, folderPath) .commit(); - assertThat(table.locationProvider().newDataLocation("file")) - .as("folder storage path should be used when set") - .contains(folderPath); - - String dataPath = "s3://random/data/location"; - table.updateProperties().set(TableProperties.WRITE_DATA_LOCATION, dataPath).commit(); - - assertThat(table.locationProvider().newDataLocation("file")) - .as("write data path should be used when set") - .contains(dataPath); + assertThatThrownBy(() -> table.locationProvider().newDataLocation("file")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage( + "Property 'write.folder-storage.path' has been deprecated and will be removed in 2.0, use 'write.data.path' instead."); } @TestTemplate diff --git a/docs/docs/aws.md b/docs/docs/aws.md index 2111793a160a..29aa8549cffc 100644 --- a/docs/docs/aws.md +++ b/docs/docs/aws.md @@ -385,6 +385,7 @@ Note, the path resolution logic for `ObjectStoreLocationProvider` is `write.data However, for the older versions up to 0.12.0, the logic is as follows: - before 0.12.0, `write.object-storage.path` must be set. - at 0.12.0, `write.object-storage.path` then `write.folder-storage.path` then `/data`. +- at 2.0.0 `write.object-storage.path` and `write.folder-storage.path` will be removed For more details, please refer to the [LocationProvider Configuration](custom-catalog.md#custom-location-provider-implementation) section.