Skip to content

Core: rename WRITE_NEW_DATA_LOCATION to WRITE_FOLDER_STORAGE_LOCATION #2965

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

Merged
rdblue merged 2 commits into from
Aug 23, 2021
Merged

Core: rename WRITE_NEW_DATA_LOCATION to WRITE_FOLDER_STORAGE_LOCATION #2965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41c9702
Core: rename WRITE_NEW_DATA_LOCATION to WRITE_FOLDER_STORAGE_LOCATION
Aug 13, 2021
c7b8afa
fix tests
Aug 13, 2021
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
3 changes: 2 additions & 1 deletion 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 @@ -69,7 +69,8 @@ public static LocationProvider locationsFor(String location, Map<String, String>
}

private static String defaultDataLocation(String tableLocation, Map<String, String> properties) {
return properties.getOrDefault(TableProperties.WRITE_NEW_DATA_LOCATION, String.format("%s/data", tableLocation));
return properties.getOrDefault(TableProperties.WRITE_FOLDER_STORAGE_LOCATION,
String.format("%s/data", tableLocation));
}

static class DefaultLocationProvider implements LocationProvider {
Expand Down
6 changes: 6 additions & 0 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 @@ -142,6 +142,12 @@ private TableProperties() {
// 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.
public static final String WRITE_FOLDER_STORAGE_LOCATION = "write.folder-storage.path";

/**
* @deprecated will be removed in 0.14.0, use {@link #WRITE_FOLDER_STORAGE_LOCATION} instead
*/
@Deprecated
Copy link
Contributor

@kbendick kbendick Aug 13, 2021

Choose a reason for hiding this comment

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

+1 to the deprecation.

When the deprecation comes around, we should consider providing an action to migrate tables off of deprecated properties. In general, I think that would allow us to make smarter decisions about table property names as new needs surface. Something similar to the v2 table migration action. But that’s a long ways off.

Copy link
Contributor Author

@jackye1995 jackye1995 Aug 13, 2021

Choose a reason for hiding this comment

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

I think migration is unnecessary. The actual value stored is always write.folder-storage.path, we are just changing the Java variable name. And as I noted, we will deprecate after 2 releases, so that people can have time to change their variable name.

Copy link
Contributor

@flyrain flyrain Aug 19, 2021

Choose a reason for hiding this comment

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

Why will be removed in 0.14.0? Is there a convention to follow?

public static final String WRITE_NEW_DATA_LOCATION = "write.folder-storage.path";

// This only applies to files written after this property is set. Files previously written aren't
Expand Down
4 changes: 2 additions & 2 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 @@ -114,7 +114,7 @@ public void testDefaultLocationProvider() {
@Test
public void testDefaultLocationProviderWithCustomDataLocation() {
this.table.updateProperties()
.set(TableProperties.WRITE_NEW_DATA_LOCATION, "new_location")
.set(TableProperties.WRITE_FOLDER_STORAGE_LOCATION, "new_location")
Copy link
Contributor

@kbendick kbendick Aug 13, 2021

Choose a reason for hiding this comment

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

Does it make sense to leave a test for the deprecated config value?

I’d love to even log something if a user sets it, but that can be considered later (if at all).

Copy link
Contributor Author

@jackye1995 jackye1995 Aug 13, 2021

Choose a reason for hiding this comment

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

It is the same value, just using a different name to avoid confusion.

Copy link
Contributor

@kbendick kbendick Aug 19, 2021

Choose a reason for hiding this comment

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

Yeah I see now. This won't affect any external folks unless they have some custom stuff that touches this config.

.commit();

this.table.locationProvider().newDataLocation("my_file");
Expand Down Expand Up @@ -224,7 +224,7 @@ public void testObjectStorageLocationProviderPathResolution() {

String folderPath = "s3://random/folder/location";
table.updateProperties()
.set(TableProperties.WRITE_NEW_DATA_LOCATION, folderPath)
.set(TableProperties.WRITE_FOLDER_STORAGE_LOCATION, folderPath)
.commit();

Assert.assertTrue("folder storage path should be used when set",
Expand Down
5 changes: 2 additions & 3 deletions spark/src/jmh/java/org/apache/iceberg/spark/source/IcebergSourceBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;

import static org.apache.iceberg.TableProperties.WRITE_NEW_DATA_LOCATION;

@Fork(1)
@State(Scope.Benchmark)
@Warmup(iterations = 3)
Expand Down Expand Up @@ -81,7 +79,8 @@ protected String newTableLocation() {

protected String dataLocation() {
Map<String, String> properties = table.properties();
return properties.getOrDefault(WRITE_NEW_DATA_LOCATION, String.format("%s/data", table.location()));
return properties.getOrDefault(TableProperties.WRITE_FOLDER_STORAGE_LOCATION,
String.format("%s/data", table.location()));
}

protected void cleanupFiles() throws IOException {
Expand Down
4 changes: 2 additions & 2 deletions spark/src/test/java/org/apache/iceberg/actions/TestRemoveOrphanFilesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ public void testWapFilesAreKept() throws InterruptedException {
public void testMetadataFolderIsIntact() throws InterruptedException {
// write data directly to the table location
Map<String, String> props = Maps.newHashMap();
props.put(TableProperties.WRITE_NEW_DATA_LOCATION, tableLocation);
props.put(TableProperties.WRITE_FOLDER_STORAGE_LOCATION, tableLocation);
Table table = TABLES.create(SCHEMA, SPEC, props, tableLocation);

List<ThreeColumnRecord> records = Lists.newArrayList(
Expand Down Expand Up @@ -357,7 +357,7 @@ public void testOlderThanTimestamp() throws InterruptedException {
@Test
public void testRemoveUnreachableMetadataVersionFiles() throws InterruptedException {
Map<String, String> props = Maps.newHashMap();
props.put(TableProperties.WRITE_NEW_DATA_LOCATION, tableLocation);
props.put(TableProperties.WRITE_FOLDER_STORAGE_LOCATION, tableLocation);
props.put(TableProperties.METADATA_PREVIOUS_VERSIONS_MAX, "1");
Table table = TABLES.create(SCHEMA, SPEC, props, tableLocation);

Expand Down
6 changes: 3 additions & 3 deletions spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public void testWriteWithCustomDataLocation() throws IOException {
File tablePropertyDataLocation = temp.newFolder("test-table-property-data-dir");
Table table = createTable(new Schema(SUPPORTED_PRIMITIVES.fields()), location);
table.updateProperties().set(
TableProperties.WRITE_NEW_DATA_LOCATION, tablePropertyDataLocation.getAbsolutePath()).commit();
TableProperties.WRITE_FOLDER_STORAGE_LOCATION, tablePropertyDataLocation.getAbsolutePath()).commit();
writeAndValidateWithLocations(table, location, tablePropertyDataLocation);
}

Expand Down Expand Up @@ -271,7 +271,7 @@ public void testNullableWithWriteOption() throws IOException {
String sourcePath = String.format("%s/nullable_poc/sourceFolder/", location.toString());
String targetPath = String.format("%s/nullable_poc/targetFolder/", location.toString());

tableProperties = ImmutableMap.of(TableProperties.WRITE_NEW_DATA_LOCATION, targetPath);
tableProperties = ImmutableMap.of(TableProperties.WRITE_FOLDER_STORAGE_LOCATION, targetPath);

// read this and append to iceberg dataset
spark
Expand Down Expand Up @@ -312,7 +312,7 @@ public void testNullableWithSparkSqlOption() throws IOException {
String sourcePath = String.format("%s/nullable_poc/sourceFolder/", location.toString());
String targetPath = String.format("%s/nullable_poc/targetFolder/", location.toString());

tableProperties = ImmutableMap.of(TableProperties.WRITE_NEW_DATA_LOCATION, targetPath);
tableProperties = ImmutableMap.of(TableProperties.WRITE_FOLDER_STORAGE_LOCATION, targetPath);

// read this and append to iceberg dataset
spark
Expand Down
2 changes: 1 addition & 1 deletion spark3/src/main/java/org/apache/iceberg/spark/actions/BaseSnapshotTableSparkAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ protected Map<String, String> destTableProps() {
// remove any possible location properties from origin properties
properties.remove(LOCATION);
properties.remove(TableProperties.WRITE_METADATA_LOCATION);
properties.remove(TableProperties.WRITE_NEW_DATA_LOCATION);
properties.remove(TableProperties.WRITE_FOLDER_STORAGE_LOCATION);

// set default and user-provided props
properties.put(TableCatalog.PROP_PROVIDER, "iceberg");
Expand Down