From cb0201fe585eee3d02fdacbf2403be3f9e986f95 Mon Sep 17 00:00:00 2001 From: Fokko Date: Tue, 18 Feb 2025 18:48:54 +0100 Subject: [PATCH 1/5] Warn on `{write.folder-storage.path,write.object-storage.path}` properties Another way of warning non-Java users about the deprecated properties - `write.folder-storage.path` - `write.object-storage.path` This will only warn when `write.data.path` is not set. So hopefuly the number in the logs will be limited. --- .../org/apache/iceberg/LocationProviders.java | 18 ++++++++++++++++++ .../org/apache/iceberg/TableProperties.java | 4 ++-- docs/docs/aws.md | 1 + 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/LocationProviders.java b/core/src/main/java/org/apache/iceberg/LocationProviders.java index 68bec2f4e4fc..a1f0c03ee88d 100644 --- a/core/src/main/java/org/apache/iceberg/LocationProviders.java +++ b/core/src/main/java/org/apache/iceberg/LocationProviders.java @@ -29,8 +29,11 @@ import org.apache.iceberg.relocated.com.google.common.hash.Hashing; import org.apache.iceberg.util.LocationUtil; import org.apache.iceberg.util.PropertyUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class LocationProviders { + private static final Logger LOG = LoggerFactory.getLogger(LocationProviders.class); private LocationProviders() {} @@ -88,6 +91,11 @@ private static String dataLocation(Map properties, String tableL dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION); if (dataLocation == null) { dataLocation = String.format("%s/data", tableLocation); + } else { + LOG.warn( + "Property '{}' has been deprecated and will be removed in 2.0, use '{}' instead.", + TableProperties.WRITE_FOLDER_STORAGE_LOCATION, + TableProperties.WRITE_DATA_LOCATION); } } return dataLocation; @@ -142,7 +150,17 @@ private static String dataLocation(Map properties, String tableL dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION); if (dataLocation == null) { dataLocation = String.format("%s/data", tableLocation); + } else { + LOG.warn( + "Property '{}' has been deprecated and will be removed in 2.0, use '{}' instead.", + TableProperties.WRITE_FOLDER_STORAGE_LOCATION, + TableProperties.WRITE_DATA_LOCATION); } + } else { + LOG.warn( + "Property '{}' has been deprecated and will be removed in 2.0, use '{}' instead.", + TableProperties.OBJECT_STORE_PATH, + TableProperties.WRITE_DATA_LOCATION); } } return dataLocation; 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/docs/docs/aws.md b/docs/docs/aws.md index 1a98a4d18e5b..bdf08dceace1 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. From 216a04a87f0e1d358bad60c5dcd21136d22a6280 Mon Sep 17 00:00:00 2001 From: Fokko Date: Wed, 19 Feb 2025 11:54:12 +0100 Subject: [PATCH 2/5] Throw instead --- .../org/apache/iceberg/LocationProviders.java | 29 +++++---- .../apache/iceberg/TestLocationProvider.java | 63 +++++++------------ 2 files changed, 37 insertions(+), 55 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/LocationProviders.java b/core/src/main/java/org/apache/iceberg/LocationProviders.java index a1f0c03ee88d..cc5ca69f204f 100644 --- a/core/src/main/java/org/apache/iceberg/LocationProviders.java +++ b/core/src/main/java/org/apache/iceberg/LocationProviders.java @@ -29,11 +29,8 @@ import org.apache.iceberg.relocated.com.google.common.hash.Hashing; import org.apache.iceberg.util.LocationUtil; import org.apache.iceberg.util.PropertyUtil; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; public class LocationProviders { - private static final Logger LOG = LoggerFactory.getLogger(LocationProviders.class); private LocationProviders() {} @@ -92,10 +89,11 @@ private static String dataLocation(Map properties, String tableL if (dataLocation == null) { dataLocation = String.format("%s/data", tableLocation); } else { - LOG.warn( - "Property '{}' has been deprecated and will be removed in 2.0, use '{}' instead.", - TableProperties.WRITE_FOLDER_STORAGE_LOCATION, - TableProperties.WRITE_DATA_LOCATION); + throw new UnsupportedOperationException( + String.format( + "Property '%s' has been deprecated and will be removed in 2.0, use '%s' instead.", + TableProperties.WRITE_FOLDER_STORAGE_LOCATION, + TableProperties.WRITE_DATA_LOCATION)); } } return dataLocation; @@ -151,16 +149,17 @@ private static String dataLocation(Map properties, String tableL if (dataLocation == null) { dataLocation = String.format("%s/data", tableLocation); } else { - LOG.warn( - "Property '{}' has been deprecated and will be removed in 2.0, use '{}' instead.", - TableProperties.WRITE_FOLDER_STORAGE_LOCATION, - TableProperties.WRITE_DATA_LOCATION); + throw new UnsupportedOperationException( + String.format( + "Property '%s' has been deprecated and will be removed in 2.0, use '%s' instead.", + TableProperties.WRITE_FOLDER_STORAGE_LOCATION, + TableProperties.WRITE_DATA_LOCATION)); } } else { - LOG.warn( - "Property '{}' has been deprecated and will be removed in 2.0, use '{}' instead.", - TableProperties.OBJECT_STORE_PATH, - TableProperties.WRITE_DATA_LOCATION); + throw new UnsupportedOperationException( + String.format( + "Property '%s' has been deprecated and will be removed in 2.0, use '%s' instead.", + TableProperties.OBJECT_STORE_PATH, TableProperties.WRITE_DATA_LOCATION)); } } return dataLocation; diff --git a/core/src/test/java/org/apache/iceberg/TestLocationProvider.java b/core/src/test/java/org/apache/iceberg/TestLocationProvider.java index 7edba51c3d85..a5790efb9cb0 100644 --- a/core/src/test/java/org/apache/iceberg/TestLocationProvider.java +++ b/core/src/test/java/org/apache/iceberg/TestLocationProvider.java @@ -214,61 +214,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(UnsupportedOperationException.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(UnsupportedOperationException.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(UnsupportedOperationException.class) + .hasMessage( + "Property 'write.folder-storage.path' has been deprecated and will be removed in 2.0, use 'write.data.path' instead."); } @TestTemplate From 2ae5fca1f8f0a73ca4147d45efbaba57ea3ca55d Mon Sep 17 00:00:00 2001 From: Fokko Date: Wed, 19 Feb 2025 16:46:37 +0100 Subject: [PATCH 3/5] Comments, thanks Peter and Robert! --- .../org/apache/iceberg/LocationProviders.java | 46 ++++++++++--------- .../apache/iceberg/TestLocationProvider.java | 6 +-- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/LocationProviders.java b/core/src/main/java/org/apache/iceberg/LocationProviders.java index cc5ca69f204f..493c88c4614c 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 getProperty(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,17 +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 = getProperty(properties, TableProperties.WRITE_DATA_LOCATION); if (dataLocation == null) { - dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION); + dataLocation = getProperty(properties, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); if (dataLocation == null) { dataLocation = String.format("%s/data", tableLocation); - } else { - throw new UnsupportedOperationException( - String.format( - "Property '%s' has been deprecated and will be removed in 2.0, use '%s' instead.", - TableProperties.WRITE_FOLDER_STORAGE_LOCATION, - TableProperties.WRITE_DATA_LOCATION)); } } return dataLocation; @@ -141,25 +154,14 @@ static class ObjectStoreLocationProvider implements LocationProvider { } private static String dataLocation(Map properties, String tableLocation) { - String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION); + String dataLocation = getProperty(properties, TableProperties.WRITE_DATA_LOCATION); if (dataLocation == null) { - dataLocation = properties.get(TableProperties.OBJECT_STORE_PATH); + dataLocation = getProperty(properties, TableProperties.OBJECT_STORE_PATH); if (dataLocation == null) { - dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION); + dataLocation = getProperty(properties, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); if (dataLocation == null) { dataLocation = String.format("%s/data", tableLocation); - } else { - throw new UnsupportedOperationException( - String.format( - "Property '%s' has been deprecated and will be removed in 2.0, use '%s' instead.", - TableProperties.WRITE_FOLDER_STORAGE_LOCATION, - TableProperties.WRITE_DATA_LOCATION)); } - } else { - throw new UnsupportedOperationException( - String.format( - "Property '%s' has been deprecated and will be removed in 2.0, use '%s' instead.", - TableProperties.OBJECT_STORE_PATH, TableProperties.WRITE_DATA_LOCATION)); } } return dataLocation; diff --git a/core/src/test/java/org/apache/iceberg/TestLocationProvider.java b/core/src/test/java/org/apache/iceberg/TestLocationProvider.java index a5790efb9cb0..53e00bb12cf3 100644 --- a/core/src/test/java/org/apache/iceberg/TestLocationProvider.java +++ b/core/src/test/java/org/apache/iceberg/TestLocationProvider.java @@ -223,7 +223,7 @@ public void testObjectStorageLocationProviderThrowOnDeprecatedProperties() { .commit(); assertThatThrownBy(() -> table.locationProvider().newDataLocation("file")) - .isInstanceOf(UnsupportedOperationException.class) + .isInstanceOf(IllegalArgumentException.class) .hasMessage( "Property 'write.folder-storage.path' has been deprecated and will be removed in 2.0, use 'write.data.path' instead."); @@ -234,7 +234,7 @@ public void testObjectStorageLocationProviderThrowOnDeprecatedProperties() { .commit(); assertThatThrownBy(() -> table.locationProvider().newDataLocation("file")) - .isInstanceOf(UnsupportedOperationException.class) + .isInstanceOf(IllegalArgumentException.class) .hasMessage( "Property 'write.object-storage.path' has been deprecated and will be removed in 2.0, use 'write.data.path' instead."); } @@ -249,7 +249,7 @@ public void testDefaultStorageLocationProviderThrowOnDeprecatedProperties() { .commit(); assertThatThrownBy(() -> table.locationProvider().newDataLocation("file")) - .isInstanceOf(UnsupportedOperationException.class) + .isInstanceOf(IllegalArgumentException.class) .hasMessage( "Property 'write.folder-storage.path' has been deprecated and will be removed in 2.0, use 'write.data.path' instead."); } From bdd63f53237de6e82f359f387e4ef078e7ed1d16 Mon Sep 17 00:00:00 2001 From: Fokko Date: Wed, 19 Feb 2025 20:17:35 +0100 Subject: [PATCH 4/5] Rename to `getAndCheckLegacyLocation` --- .../java/org/apache/iceberg/LocationProviders.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/LocationProviders.java b/core/src/main/java/org/apache/iceberg/LocationProviders.java index 493c88c4614c..75da58bdef53 100644 --- a/core/src/main/java/org/apache/iceberg/LocationProviders.java +++ b/core/src/main/java/org/apache/iceberg/LocationProviders.java @@ -81,7 +81,7 @@ public static LocationProvider locationsFor( ImmutableSet.of( TableProperties.OBJECT_STORE_PATH, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); - private static String getProperty(Map properties, String key) { + private static String getAndCheckLegacyLocation(Map properties, String key) { String value = properties.get(key); if (value != null && DEPRECATED_PROPERTIES.contains(key)) { @@ -102,9 +102,10 @@ static class DefaultLocationProvider implements LocationProvider { } private static String dataLocation(Map properties, String tableLocation) { - String dataLocation = getProperty(properties, TableProperties.WRITE_DATA_LOCATION); + String dataLocation = getAndCheckLegacyLocation(properties, TableProperties.WRITE_DATA_LOCATION); if (dataLocation == null) { - dataLocation = getProperty(properties, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); + dataLocation = + getAndCheckLegacyLocation(properties, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); if (dataLocation == null) { dataLocation = String.format("%s/data", tableLocation); } @@ -154,11 +155,12 @@ static class ObjectStoreLocationProvider implements LocationProvider { } private static String dataLocation(Map properties, String tableLocation) { - String dataLocation = getProperty(properties, TableProperties.WRITE_DATA_LOCATION); + String dataLocation = getAndCheckLegacyLocation(properties, TableProperties.WRITE_DATA_LOCATION); if (dataLocation == null) { - dataLocation = getProperty(properties, TableProperties.OBJECT_STORE_PATH); + dataLocation = getAndCheckLegacyLocation(properties, TableProperties.OBJECT_STORE_PATH); if (dataLocation == null) { - dataLocation = getProperty(properties, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); + dataLocation = + getAndCheckLegacyLocation(properties, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); if (dataLocation == null) { dataLocation = String.format("%s/data", tableLocation); } From 7d0cc6700e6149b31b5fb3fcd32007e58f46bbb7 Mon Sep 17 00:00:00 2001 From: Fokko Date: Wed, 19 Feb 2025 21:53:08 +0100 Subject: [PATCH 5/5] Spotless --- .../src/main/java/org/apache/iceberg/LocationProviders.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/LocationProviders.java b/core/src/main/java/org/apache/iceberg/LocationProviders.java index 75da58bdef53..6a75529ddb91 100644 --- a/core/src/main/java/org/apache/iceberg/LocationProviders.java +++ b/core/src/main/java/org/apache/iceberg/LocationProviders.java @@ -102,7 +102,8 @@ static class DefaultLocationProvider implements LocationProvider { } private static String dataLocation(Map properties, String tableLocation) { - String dataLocation = getAndCheckLegacyLocation(properties, TableProperties.WRITE_DATA_LOCATION); + String dataLocation = + getAndCheckLegacyLocation(properties, TableProperties.WRITE_DATA_LOCATION); if (dataLocation == null) { dataLocation = getAndCheckLegacyLocation(properties, TableProperties.WRITE_FOLDER_STORAGE_LOCATION); @@ -155,7 +156,8 @@ static class ObjectStoreLocationProvider implements LocationProvider { } private static String dataLocation(Map properties, String tableLocation) { - String dataLocation = getAndCheckLegacyLocation(properties, TableProperties.WRITE_DATA_LOCATION); + String dataLocation = + getAndCheckLegacyLocation(properties, TableProperties.WRITE_DATA_LOCATION); if (dataLocation == null) { dataLocation = getAndCheckLegacyLocation(properties, TableProperties.OBJECT_STORE_PATH); if (dataLocation == null) {