kbendick commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r705748443
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -141,4 +133,18 @@ private static String stripTrailingSlash(String path) {
}
return result;
}
+
+ public static String dataLocation(Map<String, String> properties, String
tableLocation) {
Review comment:
Nit: Does this need to be `public` or can it be `private`? If it needs
to be visible for testing or something, that's ok.
##########
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: Do we want to update the comment for the default if not set to
reflect anything about the possibility of object storage location provider?
Up to you. The more I think about it, the more I think it just complicates
things and that we should just properly document the behavior on the website.
But up to you.
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -141,4 +133,18 @@ private static String stripTrailingSlash(String path) {
}
return result;
}
+
+ public 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);
+ }
+ }
+ }
+ return dataLocation;
+ }
Review comment:
While we're falling back through several deprecated options, we might
want to list out the intended fallback order / behavior (in a comment up top
for example) so we can more easily verify that it happens.
Or better yet, add tests setting different combinations.
We should potentially also address whether or not it's possible for users to
have set too many flags and then error out.
Lastly, might want to drop a warning level deprecation log if one of the
deprecated flags is non-null (or maybe just info). :)
--
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]
