You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/09/09 18:08:01 UTC
[GitHub] [iceberg] flyrain opened a new pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
flyrain opened a new pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094
Per discussion in #2965, we deprecate both `write.folder-storage.path` and `write.object-storage.path`, and use `write.data.path` instead.
Created a new method `LocationProviders::dataLocation` to get the data location and to be compatible with deprecated configs.
cc @rdblue @jackye1995 @aokolnychyi @RussellSpitzer @kbendick @karuppayya
--
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
[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
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r711363026
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -141,4 +136,41 @@ private static String stripTrailingSlash(String path) {
}
return result;
}
+
+ /**
+ * Get the data file location. For the {@link DefaultLocationProvider}, the priority level are
+ * "write.data.path" -> "write.folder-storage.path" -> "table-location/data".
+ * For the {@link ObjectStoreLocationProvider}, the priority level are
+ * "write.data.path" -> "write.object-storage.path" -> "write.folder-storage.path" -> "table-location/data".
+ */
+ private static String dataLocation(Map<String, String> properties, String tableLocation, boolean isObjectStore) {
Review comment:
Nit / non-blocking: Naming this `isObjectStorageLocationProvider` or something to indicate that it's not just object storage, but specifically the usage of object storage location provider.
However, since I just realized this is already in `LocationProviders.java`, I think this is probably fine and no need to change (especially as it's documented in the java doc).
--
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
[GitHub] [iceberg] flyrain edited a comment on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain edited a comment on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-923318367
Hi @rdblue, I like your idea to simplify the logic, but as @jackye1995 mentioned, the logic to use write.folder-storage.path in the object store location provider was introduced by #2845, which has been released in 0.12.0. #2965 is a PR to just change the constant name.
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r708876536
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -141,4 +138,34 @@ private static String stripTrailingSlash(String path) {
}
return result;
}
+
+ private static String dataLocation(Map<String, String> properties, String tableLocation, boolean isObjectStore) {
+ String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION);
+ if (dataLocation == null) {
+ String deprecatedProperty = null;
Review comment:
I think we can use `String deprecatedProperty = isObjectStorage ? TableProperties.OBJECT_STORE_PATH : TableProperties.WRITE_FOLDER_STORAGE_LOCATION`, then `deprecatedProperty` is always not null, and you don't need to do null check for the warning, and we can avoid having that warning helper method.
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r707778397
##########
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:
Yeah, the logic is in #2845, and has been in the release 0.12.0. We'd better to stick to it. I will make the change. cc @rdblue.
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r707686439
##########
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:
Hi @jackye1995 , thanks for the information. I've added both `WRITE_FOLDER_STORAGE_LOCATION` and `OBJECT_STORE_PATH`. Can you rebase it in PR #2966 once this got 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: 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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r707544746
##########
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:
It'd be safer to remove both. Added in the new commit.
--
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
[GitHub] [iceberg] rdblue merged pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094
--
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
[GitHub] [iceberg] flyrain commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-916539628
> Clarifying for my own understanding, but if user enables object-storage location provider and _doesnt_ set any of these paths, what happens?
>
> Like in this case, does the data file still get the hash? And if so, where is it placed? `table.location() + "/data" + <hash>`?
This PR doesn't change the behavior of object storage location provider. It will fall back to `table.location() + "/data"` if no config is set.
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r706372410
##########
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:
Added the log.
--
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
[GitHub] [iceberg] rdblue commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-923243388
Looks like the change that defaulted object storage tables to the folder-storage path has not been released yet: https://github.com/apache/iceberg/commit/aef12c0e702ef8c306826581893963e3dc5f6ba3
@flyrain and @jackye1995, how about simplifying this by only using `write.data.path` and removing the use of `write.folder-storage.path` from the object store location provider? That simplifies the documentation and makes it easier to understand.
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r709507046
##########
File path: site/docs/aws.md
##########
@@ -373,8 +373,7 @@ s3://my-table-data-bucket/2d3905f8/my_ns.db/my_table/category=orders/00000-0-5af
```
Note, the path resolution logic for `ObjectStoreLocationProvider` is as follows:
-- if `write.object-storage.path` is set, use it
-- if not found, fallback to `write.folder-storage.path`
+- if `write.data.path` is set, use it
- if not found, use `<tableLocation>/data`
Review comment:
Given we have a simple path resolution strategy, I think we can just put these listing in a single sentence. Also add 2 warning blocks describing the legacy behaviors:
- 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`
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r707722371
##########
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:
yeah I would agree with that if the change is not in 0.12.0, but unfortunately that PR is a part of the 0.12.0 release. https://github.com/apache/iceberg/compare/apache-iceberg-0.12.0...master
--
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
[GitHub] [iceberg] jun-he edited a comment on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
jun-he edited a comment on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-922102334
> Hi @jun-he, do we need a similar change in python? table_properties.py contains WRITE_NEW_DATA_LOCATION and WRITE_METADATA_LOCATION as well. I'm glad to add it if the change is needed. Another option is to handle it in a follow-up PR.
@flyrain thanks for pinging. Yep, we need to have the similar changes. But I think it is fine not to add it in the python_legacy. Instead, we can add it to the new python library. I am currently working on its layout with a high level design. Once that is done, we will start the implementation and then add it to python. I will create an issue for us to track it.
Additionally, wondering if we still need to support those deprecated fields for backward compatibility?
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r705813395
##########
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:
`testObjectStorageLocationProviderPathResolution` is for that. Will add warn log.
--
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
[GitHub] [iceberg] flyrain commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-918583021
> nit: we can move L161 to after L144 to avoid deleting the comments and add again here.
I am trying to put these two guys together.
```
// 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";
// 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 "metadata" folder underneath the root path of the table.
public static final String WRITE_METADATA_LOCATION = "write.metadata.path";
```
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r707678903
##########
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:
yeah, I was using the formatting tool for the whole file. Will remove it.
--
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
[GitHub] [iceberg] jun-he commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
jun-he commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-922102334
> Hi @jun-he, do we need a similar change in python? table_properties.py contains WRITE_NEW_DATA_LOCATION and WRITE_METADATA_LOCATION as well. I'm glad to add it if the change is needed. Another option is to handle it in a follow-up PR.
@flyrain thanks for pinging. Yep, we need to have the similar changes. But I think it is fine not to add it in the python_legacy. Instead, we can add it to the new python library. I am currently working on its layout with a high level design. Once that is done, we will start the implementation and then add it to python. I will create an issue for us to track it.
Additionally, wondering if we still need to support those deprecated fields for backward compatibility?
--
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
[GitHub] [iceberg] rdblue commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-925170386
I don't think that we shouldn't make a breaking behavior change at 1.0. It isn't important enough to clean this up to warrant a breaking change. Let's just continue with the released behavior as a fallback.
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r706372155
##########
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);
Review comment:
Agreed. Made the change. This is going to be a little different from #2965. Modified the test case `testObjectStorageLocationProviderPathResolution` and added `testDefaultStorageLocationProviderPathResolution`.
--
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
[GitHub] [iceberg] flyrain commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-918420049
Hi @jun-he, do we need a similar change in python? table_properties.py contains WRITE_NEW_DATA_LOCATION and WRITE_METADATA_LOCATION as well. I'm glad to add it if the change is needed. Another option is to handle it in a follow-up PR.
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r707679071
##########
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:
Will remove it.
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r708872084
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -99,8 +96,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, true));
Review comment:
nit: is this newline needed?
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r709416782
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -141,4 +138,34 @@ private static String stripTrailingSlash(String path) {
}
return result;
}
+
+ private static String dataLocation(Map<String, String> properties, String tableLocation, boolean isObjectStore) {
+ String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION);
+ if (dataLocation == null) {
+ String deprecatedProperty = null;
Review comment:
Made the change. Thanks for the suggestion.
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r707718407
##########
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:
Sure, I can do that
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r708871961
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -68,16 +69,12 @@ public static LocationProvider locationsFor(String location, Map<String, String>
}
}
- private static String defaultDataLocation(String tableLocation, Map<String, String> properties) {
- return properties.getOrDefault(TableProperties.WRITE_FOLDER_STORAGE_LOCATION,
- String.format("%s/data", tableLocation));
- }
-
static class DefaultLocationProvider implements LocationProvider {
private final String dataLocation;
DefaultLocationProvider(String tableLocation, Map<String, String> properties) {
- this.dataLocation = stripTrailingSlash(defaultDataLocation(tableLocation, properties));
+ this.dataLocation =
+ stripTrailingSlash(dataLocation(properties, tableLocation, false));
Review comment:
nit: is this newline needed?
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r707778397
##########
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:
Yeah, the logic of #2845 has been in the release 0.12.0. We'd better to stick to it. I will make the change. cc @rdblue.
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r705800916
##########
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
Review comment:
I doubt this will ever be removed. It isn't worth breaking on older tables.
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r707680350
##########
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:
That's my original logic. I made the change per Ryan's comments https://github.com/apache/iceberg/pull/3094#discussion_r705800767, which makes sense to me.
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r706371280
##########
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
Review comment:
Make sense, remove the comments.
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r709630036
##########
File path: site/docs/aws.md
##########
@@ -373,8 +373,7 @@ s3://my-table-data-bucket/2d3905f8/my_ns.db/my_table/category=orders/00000-0-5af
```
Note, the path resolution logic for `ObjectStoreLocationProvider` is as follows:
-- if `write.object-storage.path` is set, use it
-- if not found, fallback to `write.folder-storage.path`
+- if `write.data.path` is set, use it
- if not found, use `<tableLocation>/data`
Review comment:
Thanks Jack! Made the change for the doc.
--
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
[GitHub] [iceberg] jackye1995 commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-924629441
@rdblue @flyrain
I think if 1.0.0 is the next release, what @rdblue proposes is definitely the right approach to go, and we have also a way out for users of 0.12.0, which is great.
If we are still thinking about 0.13.0, we need to just be more cautious, because these config keys will be there all the time, I think it's better to keep the resolution strategy. We decided to merge #2845 mostly because services try to hide the table's root location, but the object storage mode forces user to configure a location that the user might not know. Chaining the fallback config key seems to be the only way out for the 2 use cases described here: https://github.com/apache/iceberg/pull/2845#issuecomment-897900861
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
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: 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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r708734300
##########
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:
Hi @jackye1995, made the logic to be the same with #2845. The implementation looks not as clean as before, but that's probably the best thing we can do.
--
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
[GitHub] [iceberg] flyrain commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-923318367
Hi @rdblue, I like your idea, but as @jackye1995 mentioned, the logic to use write.folder-storage.path in the object store location provider was introduced by #2845, which has been released in 0.12.0. #2965 is a PR to just change the constant name.
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r711824677
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -141,4 +136,41 @@ private static String stripTrailingSlash(String path) {
}
return result;
}
+
+ /**
+ * Get the data file location. For the {@link DefaultLocationProvider}, the priority level are
+ * "write.data.path" -> "write.folder-storage.path" -> "table-location/data".
+ * For the {@link ObjectStoreLocationProvider}, the priority level are
+ * "write.data.path" -> "write.object-storage.path" -> "write.folder-storage.path" -> "table-location/data".
+ */
+ private static String dataLocation(Map<String, String> properties, String tableLocation, boolean isObjectStore) {
+ String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION);
+ if (dataLocation == null) {
+ dataLocation = deprecatedDataLocation(properties, isObjectStore);
+ if (dataLocation == null) {
+ dataLocation = String.format("%s/data", tableLocation);
+ }
+ }
+ return dataLocation;
+ }
+
+ private static String deprecatedDataLocation(Map<String, String> properties, boolean isObjectStore) {
+ String deprecatedProperty = isObjectStore ?
+ TableProperties.OBJECT_STORE_PATH : TableProperties.WRITE_FOLDER_STORAGE_LOCATION;
+
+ String dataLocation = properties.get(deprecatedProperty);
+
+ final String warnMsg = "Table property {} is deprecated, please use " + TableProperties.WRITE_DATA_LOCATION +
+ " instead.";
+ if (dataLocation != null) {
+ LOG.warn(warnMsg, deprecatedProperty);
+ } else if (deprecatedProperty.equals(TableProperties.OBJECT_STORE_PATH)) {
+ dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
+ if (dataLocation != null) {
+ LOG.warn(warnMsg, TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
+ }
+ }
+
+ return dataLocation;
Review comment:
This looks way too complicated to be worth it. Let's just duplicate the logic and minimize the changes in each location provider.
--
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
[GitHub] [iceberg] rdblue commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-925171908
Thanks, @flyrain!
--
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
[GitHub] [iceberg] flyrain commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-925206854
Thanks all for the review and commit. @rdblue @jackye1995 @kbendick @karuppayya @jun-he.
Hi @anuragmantri, fyi, this will affect the relative path design a bit.
--
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
[GitHub] [iceberg] karuppayya commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
karuppayya commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r706981047
##########
File path: core/src/test/java/org/apache/iceberg/TestLocationProvider.java
##########
@@ -222,6 +222,32 @@ public void testObjectStorageLocationProviderPathResolution() {
Assert.assertTrue("default data location should be used when object storage path not set",
table.locationProvider().newDataLocation("file").contains(table.location() + "/data"));
+ String objectPath = "s3://random/object/location";
+ table.updateProperties()
+ .set(TableProperties.OBJECT_STORE_PATH, objectPath)
+ .commit();
+
+ Assert.assertTrue("object storage path should be used when set",
+ table.locationProvider().newDataLocation("file").contains(objectPath));
+
+ String dataPath = "s3://random/data/location";
+ table.updateProperties()
+ .set(TableProperties.WRITE_DATA_LOCATION, dataPath)
+ .commit();
+
+ Assert.assertTrue("object storage path should be used when set",
Review comment:
nit: should this be `write data should be used when set`?
##########
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:
When snapshotting a table that has folder storage location set, should we also remove it in addition to write data location?
Also should we remove object storage location?
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r706373556
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -141,4 +140,20 @@ private static String stripTrailingSlash(String path) {
}
return result;
}
+
+ private static String dataLocation(Map<String, String> properties, String tableLocation, String deprecatedProperty) {
+ String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION);
+ if (dataLocation == null) {
+ dataLocation = properties.get(deprecatedProperty);
Review comment:
We don't have to check if deprecatedProperty is null, the map can take a null.
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r711824701
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -141,4 +136,41 @@ private static String stripTrailingSlash(String path) {
}
return result;
}
+
+ /**
+ * Get the data file location. For the {@link DefaultLocationProvider}, the priority level are
+ * "write.data.path" -> "write.folder-storage.path" -> "table-location/data".
+ * For the {@link ObjectStoreLocationProvider}, the priority level are
+ * "write.data.path" -> "write.object-storage.path" -> "write.folder-storage.path" -> "table-location/data".
+ */
+ private static String dataLocation(Map<String, String> properties, String tableLocation, boolean isObjectStore) {
+ String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION);
+ if (dataLocation == null) {
+ dataLocation = deprecatedDataLocation(properties, isObjectStore);
+ if (dataLocation == null) {
+ dataLocation = String.format("%s/data", tableLocation);
+ }
+ }
+ return dataLocation;
+ }
+
+ private static String deprecatedDataLocation(Map<String, String> properties, boolean isObjectStore) {
+ String deprecatedProperty = isObjectStore ?
+ TableProperties.OBJECT_STORE_PATH : TableProperties.WRITE_FOLDER_STORAGE_LOCATION;
+
+ String dataLocation = properties.get(deprecatedProperty);
+
+ final String warnMsg = "Table property {} is deprecated, please use " + TableProperties.WRITE_DATA_LOCATION +
+ " instead.";
+ if (dataLocation != null) {
+ LOG.warn(warnMsg, deprecatedProperty);
+ } else if (deprecatedProperty.equals(TableProperties.OBJECT_STORE_PATH)) {
+ dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
+ if (dataLocation != null) {
+ LOG.warn(warnMsg, TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
Review comment:
No need for warnings. This is not important enough to nag users about.
--
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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r712399764
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -141,4 +136,41 @@ private static String stripTrailingSlash(String path) {
}
return result;
}
+
+ /**
+ * Get the data file location. For the {@link DefaultLocationProvider}, the priority level are
+ * "write.data.path" -> "write.folder-storage.path" -> "table-location/data".
+ * For the {@link ObjectStoreLocationProvider}, the priority level are
+ * "write.data.path" -> "write.object-storage.path" -> "write.folder-storage.path" -> "table-location/data".
+ */
+ private static String dataLocation(Map<String, String> properties, String tableLocation, boolean isObjectStore) {
+ String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION);
+ if (dataLocation == null) {
+ dataLocation = deprecatedDataLocation(properties, isObjectStore);
+ if (dataLocation == null) {
+ dataLocation = String.format("%s/data", tableLocation);
+ }
+ }
+ return dataLocation;
+ }
+
+ private static String deprecatedDataLocation(Map<String, String> properties, boolean isObjectStore) {
+ String deprecatedProperty = isObjectStore ?
+ TableProperties.OBJECT_STORE_PATH : TableProperties.WRITE_FOLDER_STORAGE_LOCATION;
+
+ String dataLocation = properties.get(deprecatedProperty);
+
+ final String warnMsg = "Table property {} is deprecated, please use " + TableProperties.WRITE_DATA_LOCATION +
+ " instead.";
+ if (dataLocation != null) {
+ LOG.warn(warnMsg, deprecatedProperty);
+ } else if (deprecatedProperty.equals(TableProperties.OBJECT_STORE_PATH)) {
+ dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
+ if (dataLocation != null) {
+ LOG.warn(warnMsg, TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
Review comment:
Removed
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -141,4 +136,41 @@ private static String stripTrailingSlash(String path) {
}
return result;
}
+
+ /**
+ * Get the data file location. For the {@link DefaultLocationProvider}, the priority level are
+ * "write.data.path" -> "write.folder-storage.path" -> "table-location/data".
+ * For the {@link ObjectStoreLocationProvider}, the priority level are
+ * "write.data.path" -> "write.object-storage.path" -> "write.folder-storage.path" -> "table-location/data".
+ */
+ private static String dataLocation(Map<String, String> properties, String tableLocation, boolean isObjectStore) {
+ String dataLocation = properties.get(TableProperties.WRITE_DATA_LOCATION);
+ if (dataLocation == null) {
+ dataLocation = deprecatedDataLocation(properties, isObjectStore);
+ if (dataLocation == null) {
+ dataLocation = String.format("%s/data", tableLocation);
+ }
+ }
+ return dataLocation;
+ }
+
+ private static String deprecatedDataLocation(Map<String, String> properties, boolean isObjectStore) {
+ String deprecatedProperty = isObjectStore ?
+ TableProperties.OBJECT_STORE_PATH : TableProperties.WRITE_FOLDER_STORAGE_LOCATION;
+
+ String dataLocation = properties.get(deprecatedProperty);
+
+ final String warnMsg = "Table property {} is deprecated, please use " + TableProperties.WRITE_DATA_LOCATION +
+ " instead.";
+ if (dataLocation != null) {
+ LOG.warn(warnMsg, deprecatedProperty);
+ } else if (deprecatedProperty.equals(TableProperties.OBJECT_STORE_PATH)) {
+ dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
+ if (dataLocation != null) {
+ LOG.warn(warnMsg, TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
+ }
+ }
+
+ return dataLocation;
Review comment:
Made the change.
--
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
[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
Posted by 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
[GitHub] [iceberg] flyrain commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r705810001
##########
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:
I make it public because it is used in IcebergSourceBenchmark.java. Maybe we can change `WRITE_FOLDER_STORAGE_LOCATION` to the `WRITE_DATA_PATH` in IcebergSourceBenchmark.java?
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r705800132
##########
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:
Visible for testing should use protected or package-private.
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#discussion_r705800767
##########
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);
Review comment:
This is only correct if the table is using object storage. I think you need to update this logic to select whether to return OBJECT_STORE_PATH or WRITE_FOLDER_STORAGE_LOCATION depending on the location provider selected. Both should fall back to the table location. And the new property should take precedence.
--
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
[GitHub] [iceberg] flyrain commented on pull request #3094: Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path
Posted by GitBox <gi...@apache.org>.
flyrain commented on pull request #3094:
URL: https://github.com/apache/iceberg/pull/3094#issuecomment-922072886
Thanks all for the review. Is it ready for merging?
--
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