Skip to content

Remove deprecated OBJECT_STORE_PATH and WRITE_FOLDER_STORAGE_LOCATION #12174

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

Closed
Fokko wants to merge 4 commits into from
Closed

Remove deprecated OBJECT_STORE_PATH and WRITE_FOLDER_STORAGE_LOCATION #12174

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3da457d
Remove deprecated `WRITE_METADATA_LOCATION` and `WRITE_FOLDER_STORAGE…
Fokko Feb 4, 2025
c92da5c
Accept changes
Fokko Feb 4, 2025
6572d54
Merge branch 'main' of github.com:apache/iceberg into fd-remove-depre…
Fokko Feb 17, 2025
20af7d8
Make tests happy
Fokko Feb 17, 2025
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
6 changes: 6 additions & 0 deletions .palantir/revapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1147,6 +1147,12 @@ acceptedBreaks:
justification: "Removing deprecated code"
"1.7.0":
org.apache.iceberg:iceberg-core:
- code: "java.field.removedWithConstant"
old: "field org.apache.iceberg.TableProperties.OBJECT_STORE_PATH"
justification: "Deprecated since 0.12.0"
- code: "java.field.removedWithConstant"
old: "field org.apache.iceberg.TableProperties.WRITE_FOLDER_STORAGE_LOCATION"
justification: "Deprecated since 0.12.0"
- code: "java.method.removed"
old: "method <T extends org.apache.iceberg.StructLike> org.apache.iceberg.deletes.PositionDeleteIndex\
\ org.apache.iceberg.deletes.Deletes::toPositionIndex(java.lang.CharSequence,\
Expand Down
5 changes: 2 additions & 3 deletions aws/src/integration/java/org/apache/iceberg/aws/glue/GlueTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,8 @@ public class GlueTestBase {
static final Map<String, String> TABLE_LOCATION_PROPERTIES =
ImmutableMap.of(
TableProperties.WRITE_DATA_LOCATION, "s3://" + TEST_BUCKET_NAME + "/writeDataLoc",
TableProperties.WRITE_METADATA_LOCATION, "s3://" + TEST_BUCKET_NAME + "/writeMetaDataLoc",
TableProperties.WRITE_FOLDER_STORAGE_LOCATION,
"s3://" + TEST_BUCKET_NAME + "/writeFolderStorageLoc");
TableProperties.WRITE_METADATA_LOCATION,
"s3://" + TEST_BUCKET_NAME + "/writeMetaDataLoc");

static final String TEST_BUCKET_PATH = "s3://" + TEST_BUCKET_NAME + "/" + TEST_PATH_PREFIX;

Expand Down
5 changes: 1 addition & 4 deletions aws/src/main/java/org/apache/iceberg/aws/glue/IcebergToGlueConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,7 @@ private IcebergToGlueConverter() {}
public static final String ICEBERG_FIELD_CURRENT = "iceberg.field.current";
private static final List<String> ADDITIONAL_LOCATION_PROPERTIES =
ImmutableList.of(
TableProperties.WRITE_DATA_LOCATION,
TableProperties.WRITE_METADATA_LOCATION,
TableProperties.OBJECT_STORE_PATH,
TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
TableProperties.WRITE_DATA_LOCATION, TableProperties.WRITE_METADATA_LOCATION);

// Attempt to set additionalLocations if available on the given AWS SDK version
private static final DynMethods.UnboundMethod SET_ADDITIONAL_LOCATIONS =
Expand Down
3 changes: 1 addition & 2 deletions aws/src/test/java/org/apache/iceberg/aws/glue/TestIcebergToGlueConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ public class TestIcebergToGlueConverter {
private final Map<String, String> tableLocationProperties =
ImmutableMap.of(
TableProperties.WRITE_DATA_LOCATION, "s3://writeDataLoc",
TableProperties.WRITE_METADATA_LOCATION, "s3://writeMetaDataLoc",
TableProperties.WRITE_FOLDER_STORAGE_LOCATION, "s3://writeFolderStorageLoc");
TableProperties.WRITE_METADATA_LOCATION, "s3://writeMetaDataLoc");

@Test
public void testToDatabaseName() {
Expand Down
13 changes: 2 additions & 11 deletions core/src/main/java/org/apache/iceberg/LocationProviders.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,7 @@ static class DefaultLocationProvider implements LocationProvider {
private static String dataLocation(Map<String, String> properties, String tableLocation) {
String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION);
if (dataLocation == null) {
dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
if (dataLocation == null) {
dataLocation = String.format("%s/data", tableLocation);
}
dataLocation = String.format("%s/data", tableLocation);
}
return dataLocation;
}
Expand Down Expand Up @@ -137,13 +134,7 @@ static class ObjectStoreLocationProvider implements LocationProvider {
private static String dataLocation(Map<String, String> properties, String tableLocation) {
String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION);
if (dataLocation == null) {
dataLocation = properties.get(TableProperties.OBJECT_STORE_PATH);
if (dataLocation == null) {
dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
if (dataLocation == null) {
dataLocation = String.format("%s/data", tableLocation);
}
}
dataLocation = String.format("%s/data", tableLocation);
Comment on lines -140 to +137
Copy link
Contributor

@dramaticlly dramaticlly Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I enjoy remove deprecated method or config but it's always a bit risky. From a iceberg user perspective, if I had a running iceberg data pipeline, I might not ever going to change/move away from deprecated one.

So in a worst case scenario for a given table with OBJECT_STORE_PATH or WRITE_FOLDER_STORAGE_LOCATION is set and its value is not equal to WRITE_DATA_LOCATION, shall we abort the write to force user action?

I think current logic is ignoring the old table properties and directly write into default, I think it might be reasonable but want to know if this is intended. Maybe we can raise the awareness on dev list about the change?

Copy link
Contributor Author

@Fokko Fokko Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It has been deprecated for quite a while, but I'm happy to send out an email on the dev list.

And yes, there is some risk involved if you don't import the TableProperties.{OBJECT_STORE_PATH,WRITE_FOLDER_STORAGE_LOCATION}. This way you don't see the deprecation, and your code will still compile after the removal of the property. Let's hear what others think.

}
return dataLocation;
}
Expand Down
3 changes: 0 additions & 3 deletions core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,6 @@ public static TableMetadata replacePaths(
private static Map<String, String> updateProperties(
Map<String, String> tableProperties, String sourcePrefix, String targetPrefix) {
Map<String, String> properties = Maps.newHashMap(tableProperties);
updatePathInProperty(properties, sourcePrefix, targetPrefix, TableProperties.OBJECT_STORE_PATH);
updatePathInProperty(
properties, sourcePrefix, targetPrefix, TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
updatePathInProperty(
properties, sourcePrefix, targetPrefix, TableProperties.WRITE_DATA_LOCATION);
updatePathInProperty(
Expand Down
11 changes: 0 additions & 11 deletions core/src/main/java/org/apache/iceberg/TableProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,19 +249,8 @@ private TableProperties() {}
"write.object-storage.partitioned-paths";
public static final boolean WRITE_OBJECT_STORE_PARTITIONED_PATHS_DEFAULT = true;

/**
* @deprecated 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
public static final String WRITE_FOLDER_STORAGE_LOCATION = "write.folder-storage.path";

// This only applies to files written after this property is set. Files previously written aren't
// relocated to reflect this parameter.
// If not set, defaults to a "data" folder underneath the root path of the table.
Expand Down
27 changes: 0 additions & 27 deletions core/src/test/java/org/apache/iceberg/TestLocationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,23 +221,6 @@ public void testObjectStorageLocationProviderPathResolution() {
.as("default data location should be used when object storage path not set")
.contains(table.location() + "/data");

String folderPath = "s3://random/folder/location";
table
.updateProperties()
.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 objectPath = "s3://random/object/location";
table.updateProperties().set(TableProperties.OBJECT_STORE_PATH, objectPath).commit();

assertThat(table.locationProvider().newDataLocation("file"))
.as("object storage path should be used when set")
.contains(objectPath);

String dataPath = "s3://random/data/location";
table.updateProperties().set(TableProperties.WRITE_DATA_LOCATION, dataPath).commit();
assertThat(table.locationProvider().newDataLocation("file"))
Expand All @@ -253,16 +236,6 @@ public void testDefaultStorageLocationProviderPathResolution() {
.as("default data location should be used when object storage path not set")
.contains(table.location() + "/data");

String folderPath = "s3://random/folder/location";
table
.updateProperties()
.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();

Expand Down
5 changes: 0 additions & 5 deletions docs/docs/aws.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,11 +381,6 @@ the end.
s3://my-table-data-bucket/my_ns.db/my_table/0101/0110/1001/10110010/category=orders/00000-0-5affc076-96a4-48f2-9cd2-d5efbc9f0c94-00001.parquet
```

Note, the path resolution logic for `ObjectStoreLocationProvider` is `write.data.path` then `<tableLocation>/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 `<tableLocation>/data`.

For more details, please refer to the [LocationProvider Configuration](custom-catalog.md#custom-location-provider-implementation) section.

We have also added a new table property `write.object-storage.partitioned-paths` that if set to false(default=true), this will
Expand Down
Loading