You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dpaani (via GitHub)" <gi...@apache.org> on 2023/03/23 21:07:13 UTC
[GitHub] [iceberg] dpaani opened a new pull request, #7188: Refactoring metadata location and adding API to get data and metadata location #7187
dpaani opened a new pull request, #7188:
URL: https://github.com/apache/iceberg/pull/7188
https://github.com/apache/iceberg/issues/7187
--
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] dpaani commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "dpaani (via GitHub)" <gi...@apache.org>.
dpaani commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1159994847
##########
api/src/main/java/org/apache/iceberg/io/MetadataLocationProvider.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.io;
+
+import java.io.Serializable;
+
+/**
+ * Interface for providing metadata file locations to read/write tasks.
+ *
+ * <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
+ */
+public interface MetadataLocationProvider extends Serializable {
Review Comment:
I missed that @RussellSpitzer . Now I have added MetadataUtil class and using it. Could you please check again. Thanks
--
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] RussellSpitzer commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1150921343
##########
api/src/main/java/org/apache/iceberg/io/MetadataLocationProvider.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.io;
+
+import java.io.Serializable;
+
+/**
+ * Interface for providing metadata file locations to read/write tasks.
+ *
+ * <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
+ */
+public interface MetadataLocationProvider extends Serializable {
Review Comment:
Do we need a new interface here? Could it just belong to the LocationProvider?
--
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] dpaani commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "dpaani (via GitHub)" <gi...@apache.org>.
dpaani commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1150723328
##########
api/src/main/java/org/apache/iceberg/io/LocationProvider.java:
##########
@@ -28,6 +28,16 @@
* <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
*/
public interface LocationProvider extends Serializable {
+
+ /**
+ * Return a fully-qualified data location for a table.
Review Comment:
hi @RussellSpitzer Could you please check this PR again. Regarding your concern on default method, I see that iceberg has default method in few places - https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/io/FileIO.java#L67
--
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] dpaani commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "dpaani (via GitHub)" <gi...@apache.org>.
dpaani commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1149468683
##########
core/src/main/java/org/apache/iceberg/LocationProviders.java:
##########
@@ -167,5 +185,42 @@ private static String pathContext(String tableLocation) {
return resolvedContext;
}
+
+ @Override
+ public String dataLocation() {
+ return this.storageLocation;
+ }
+ }
+
+ static class DefaultMetadataLocationProvider implements MetadataLocationProvider {
+ private static final String METADATA_FOLDER_NAME = "metadata";
+ private String metadataLocation;
+
+ DefaultMetadataLocationProvider(String tableLocation, Map<String, String> properties) {
+ this.metadataLocation =
+ LocationUtil.stripTrailingSlash(
+ metadataLocation(
+ Preconditions.checkNotNull(tableLocation, "table location cannot be null"),
Review Comment:
Thanks @nastra I have replaced it with Preconditions.checkArgument 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] dpaani commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "dpaani (via GitHub)" <gi...@apache.org>.
dpaani commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1158815383
##########
api/src/main/java/org/apache/iceberg/io/MetadataLocationProvider.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.io;
+
+import java.io.Serializable;
+
+/**
+ * Interface for providing metadata file locations to read/write tasks.
+ *
+ * <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
+ */
+public interface MetadataLocationProvider extends Serializable {
Review Comment:
hi @RussellSpitzer I have made LocationProvider to support both data and metadata locations. Could you please re-check again. Thanks
--
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] dpaani commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "dpaani (via GitHub)" <gi...@apache.org>.
dpaani commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1151048138
##########
api/src/main/java/org/apache/iceberg/io/MetadataLocationProvider.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.io;
+
+import java.io.Serializable;
+
+/**
+ * Interface for providing metadata file locations to read/write tasks.
+ *
+ * <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
+ */
+public interface MetadataLocationProvider extends Serializable {
Review Comment:
Hi @RussellSpitzer Regarding LocationProvider APIs, they are primarily related to data location, and one of the methods that is directly related to partition specification is:
```
String newDataLocation(PartitionSpec spec, StructLike partitionData, String filename)
```
I can refactor it by creating two diff interfaces (DataLocationProvider, MetadataLocationProvider) extending LocationProvider.
Adding metadata location APIs directly in LocationProvider also risky since it wont be backward compatible.
So , I thought of having MetadataLocationProvider may be good idea and later, we can also support dynamic class loading thru table properties if required to support diff implementation.
Please advise what would be the correct approach I can follow
--
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] RussellSpitzer commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1146867666
##########
api/src/main/java/org/apache/iceberg/io/LocationProvider.java:
##########
@@ -28,6 +28,16 @@
* <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
*/
public interface LocationProvider extends Serializable {
+
+ /**
+ * Return a fully-qualified data location for a table.
Review Comment:
Redundant javadoc, for these we usually just do a one liner, no annotation
```java
/**
Returns a fully-qualified data location for a table
**/
```
But I would also argue we should be a bit more descriptive, what is a "data location"
Additionally, do we want to have a default here? Shouldn't it be illegal to not provide this method? I think the intent here is not to break old location providers, but since the new code references this method I'm not sure we can get around 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] RussellSpitzer commented on pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#issuecomment-1526101279
Tests seem to be failing here?
--
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] dpaani commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "dpaani (via GitHub)" <gi...@apache.org>.
dpaani commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1151059442
##########
api/src/main/java/org/apache/iceberg/io/LocationProvider.java:
##########
@@ -28,6 +28,19 @@
* <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
*/
public interface LocationProvider extends Serializable {
+
+ /**
+ * Returns the fully-qualified data location for a table, where data files such as Parquet, ORC,
+ * or Avro are stored. The table data location can be retrieved from the table properties input,
+ * such as 'write.data.path', or from 'write.object-storage.path' (deprecated), or from
+ * 'write.folder-storage.path' (deprecated). Refer to the implementation class of {@link
+ * LocationProvider} for more details.
+ */
+ default String dataLocation() {
+ throw new UnsupportedOperationException(
+ String.format("%s does not expose dataLocation", this.getClass().getCanonicalName()));
Review Comment:
replaced with null return type and also changed junit.
--
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] RussellSpitzer commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1159200162
##########
api/src/main/java/org/apache/iceberg/io/MetadataLocationProvider.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.io;
+
+import java.io.Serializable;
+
+/**
+ * Interface for providing metadata file locations to read/write tasks.
+ *
+ * <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
+ */
+public interface MetadataLocationProvider extends Serializable {
Review Comment:
We discussed this offline and I thought we decided on extracting metadata location to a utility class since we don't actually have a use case for users having a custom location provider for metadata yet.
--
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] dpaani commented on pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "dpaani (via GitHub)" <gi...@apache.org>.
dpaani commented on PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#issuecomment-1481917931
Hi Team, Could you please help me to review this PR. TIA
cc: @flyrain @anuragmantri @RussellSpitzer
--
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] nastra commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1147444582
##########
core/src/main/java/org/apache/iceberg/LocationProviders.java:
##########
@@ -167,5 +185,42 @@ private static String pathContext(String tableLocation) {
return resolvedContext;
}
+
+ @Override
+ public String dataLocation() {
+ return this.storageLocation;
+ }
+ }
+
+ static class DefaultMetadataLocationProvider implements MetadataLocationProvider {
+ private static final String METADATA_FOLDER_NAME = "metadata";
+ private String metadataLocation;
+
+ DefaultMetadataLocationProvider(String tableLocation, Map<String, String> properties) {
+ this.metadataLocation =
+ LocationUtil.stripTrailingSlash(
+ metadataLocation(
+ Preconditions.checkNotNull(tableLocation, "table location cannot be null"),
Review Comment:
generally it is better to use `Preconditions.checkArgument(... != null, ...)` as it is better in indicating that something wrong was passed (rather than surfacing a NPE). See also https://github.com/apache/iceberg/issues/6625 for some background
--
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
Re: [PR] Refactoring metadata location and adding API to get data and metadata location #7187 [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
URL: https://github.com/apache/iceberg/pull/7188
--
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] nastra commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1147444582
##########
core/src/main/java/org/apache/iceberg/LocationProviders.java:
##########
@@ -167,5 +185,42 @@ private static String pathContext(String tableLocation) {
return resolvedContext;
}
+
+ @Override
+ public String dataLocation() {
+ return this.storageLocation;
+ }
+ }
+
+ static class DefaultMetadataLocationProvider implements MetadataLocationProvider {
+ private static final String METADATA_FOLDER_NAME = "metadata";
+ private String metadataLocation;
+
+ DefaultMetadataLocationProvider(String tableLocation, Map<String, String> properties) {
+ this.metadataLocation =
+ LocationUtil.stripTrailingSlash(
+ metadataLocation(
+ Preconditions.checkNotNull(tableLocation, "table location cannot be null"),
Review Comment:
generally it is better to use `Preconditions.checkArgument(... != null, ...)` as it is more descriptive in indicating that something wrong was passed (rather than surfacing a NPE). See also https://github.com/apache/iceberg/issues/6625 for some background
--
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] RussellSpitzer commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1150923313
##########
api/src/main/java/org/apache/iceberg/io/LocationProvider.java:
##########
@@ -28,6 +28,19 @@
* <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
*/
public interface LocationProvider extends Serializable {
+
+ /**
+ * Returns the fully-qualified data location for a table, where data files such as Parquet, ORC,
+ * or Avro are stored. The table data location can be retrieved from the table properties input,
+ * such as 'write.data.path', or from 'write.object-storage.path' (deprecated), or from
+ * 'write.folder-storage.path' (deprecated). Refer to the implementation class of {@link
+ * LocationProvider} for more details.
+ */
+ default String dataLocation() {
+ throw new UnsupportedOperationException(
+ String.format("%s does not expose dataLocation", this.getClass().getCanonicalName()));
Review Comment:
This breaks old location providers, they just would throw UnsupportedOperationException instead of MethodNotFound exceptions.
If we want the method to be backwards compatible it must return a signal instead (like null) and the rest of the code should treat that as iff this method was not implemented.
--
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
Re: [PR] Refactoring metadata location and adding API to get data and metadata location #7187 [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#issuecomment-2313786098
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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
Re: [PR] Refactoring metadata location and adding API to get data and metadata location #7187 [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#issuecomment-2330357154
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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] dpaani commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "dpaani (via GitHub)" <gi...@apache.org>.
dpaani commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1159996108
##########
api/src/main/java/org/apache/iceberg/io/LocationProvider.java:
##########
@@ -28,6 +28,19 @@
* <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
*/
public interface LocationProvider extends Serializable {
+
+ /**
+ * Returns the fully-qualified data location for a table, where data files such as Parquet, ORC,
+ * or Avro are stored. The table data location can be retrieved from the table properties input,
+ * such as 'write.data.path', or from 'write.object-storage.path' (deprecated), or from
+ * 'write.folder-storage.path' (deprecated). Refer to the implementation class of {@link
+ * LocationProvider} for more details.
+ */
+ default String dataLocation() {
+ throw new UnsupportedOperationException(
+ String.format("%s does not expose dataLocation", this.getClass().getCanonicalName()));
Review Comment:
hi @RussellSpitzer As discussed, I removed dataLocation() new API from this PR and will add it as separate 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] dpaani commented on a diff in pull request #7188: Refactoring metadata location and adding API to get data and metadata location #7187
Posted by "dpaani (via GitHub)" <gi...@apache.org>.
dpaani commented on code in PR #7188:
URL: https://github.com/apache/iceberg/pull/7188#discussion_r1149472198
##########
api/src/main/java/org/apache/iceberg/io/LocationProvider.java:
##########
@@ -28,6 +28,16 @@
* <p>Implementations must be {@link Serializable} because instances will be serialized to tasks.
*/
public interface LocationProvider extends Serializable {
+
+ /**
+ * Return a fully-qualified data location for a table.
Review Comment:
Yes @RussellSpitzer I intent to not break custom location providers. There is noway I can provide default implementation also, thats why I am throwing exception. Could you please recommend a right approach to follow here.
I added descriptive javadoc for both LocationProvider and MetadataLocationProvider. Thanks
--
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