[GitHub] [iceberg] kbendick commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org