jackye1995 commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r707596438
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -56,10 +57,12 @@ public static LocationProvider locationsFor(String
location, Map<String, String>
return ctor.newInstance(location, properties);
} catch (ClassCastException e) {
throw new IllegalArgumentException(
- String.format("Provided implementation for dynamic instantiation
should implement %s.",
+ String.format(
Review comment:
nit: no need for the newline
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -56,10 +57,12 @@ public static LocationProvider locationsFor(String
location, Map<String, String>
return ctor.newInstance(location, properties);
} catch (ClassCastException e) {
throw new IllegalArgumentException(
- String.format("Provided implementation for dynamic instantiation
should implement %s.",
+ String.format(
+ "Provided implementation for dynamic instantiation should
implement %s.",
LocationProvider.class), e);
}
- } else if (PropertyUtil.propertyAsBoolean(properties,
+ } else if (PropertyUtil.propertyAsBoolean(
Review comment:
nit: no need for the newline
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -99,8 +98,8 @@ public String newDataLocation(String filename) {
private final String context;
ObjectStoreLocationProvider(String tableLocation, Map<String, String>
properties) {
- this.storageLocation =
stripTrailingSlash(properties.getOrDefault(OBJECT_STORE_PATH,
- defaultDataLocation(tableLocation, properties)));
+ this.storageLocation =
+ stripTrailingSlash(dataLocation(properties, tableLocation,
TableProperties.OBJECT_STORE_PATH));
Review comment:
My thought is that we should fallback configs in the order of:
1. write.data.path
2. write.object-storage.path
3. write.folder-storage.path
4. default table location/data
The current implementation seems to skip 3
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -135,21 +135,31 @@ private TableProperties() {
public static final String OBJECT_STORE_ENABLED =
"write.object-storage.enabled";
public static final boolean OBJECT_STORE_ENABLED_DEFAULT = false;
+ /**
+ * @deprecated will be removed in 0.14.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";
- // 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.
+ /**
+ * @deprecated will be removed in 0.14.0, use {@link #WRITE_DATA_LOCATION}
instead
+ */
+ @Deprecated
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 will be removed in 0.14.0, use {@link #WRITE_DATA_LOCATION}
instead
*/
@Deprecated
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
+ // 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_DATA_LOCATION = "write.data.path";
Review comment:
nit: we can move L161 to after L144 to avoid deleting the comments and
add again here.
##########
File path:
spark3/src/main/java/org/apache/iceberg/spark/actions/BaseSnapshotTableSparkAction.java
##########
@@ -167,7 +167,7 @@ public SnapshotTable tableProperty(String property, String
value) {
// remove any possible location properties from origin properties
properties.remove(LOCATION);
properties.remove(TableProperties.WRITE_METADATA_LOCATION);
- properties.remove(TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
+ properties.remove(TableProperties.WRITE_DATA_LOCATION);
Review comment:
I have a PR #2966 for this change, you can either also port my tests
here, or remove this and I will update that PR once this is merged.
--
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]
