RussellSpitzer commented on a change in pull request #2658:
URL: https://github.com/apache/iceberg/pull/2658#discussion_r660000677
##########
File path: core/src/main/java/org/apache/iceberg/ManifestLists.java
##########
@@ -43,22 +47,54 @@ private ManifestLists() {
.reuseContainers(false)
.build()) {
- return Lists.newLinkedList(files);
+ return shouldUseRelativePaths ?
updateManifestFilePathsToAbsolutePaths(files,
Review comment:
I believe this was commented on the issue as well, but shouldn't we
always call updateManifestFilePathstoAbsolute here? Instead of passing through
a boolean we could just see if we need to update any files and return the
updated set? So a use with only absolute paths would essentially just pass
through the list and not change anything?
This would remove our requirement to pass through "shouldUseRelativePaths"
##########
File path: core/src/main/java/org/apache/iceberg/MetadataPathUtils.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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;
+
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.util.PropertyUtil;
+
+import static
org.apache.iceberg.TableProperties.WRITE_METADATA_USE_RELATIVE_PATH;
+import static
org.apache.iceberg.TableProperties.WRITE_METADATA_USE_RELATIVE_PATH_DEFAULT;
+
+/**
+ * Utility class that contains path conversion methods.
+ */
+public final class MetadataPathUtils {
+
+ private MetadataPathUtils() {
+ }
+
+ /**
+ * Convert a given relative path to absolute path for the table by appending
the base table location
+ * @param path relative path to be converted
+ * @param tableLocation base table location
+ * @param shouldUseRelativePaths whether relative paths should be used
+ * @return the absolute path
+ */
+ public static String toAbsolutePath(String path, String tableLocation,
boolean shouldUseRelativePaths) {
+ Preconditions.checkArgument(path != null && path.trim().length() > 0);
+ // TODO: Fix this after tests are changed to always pass the table
location. Table location cannot be null.
+ if (tableLocation == null) {
+ return path;
+ }
+ // convert to absolute path by appending the table location
+ return shouldUseRelativePaths && !path.startsWith(tableLocation) ?
tableLocation + "/" + path : path;
Review comment:
Paths.get?
##########
File path: core/src/main/java/org/apache/iceberg/MetadataPathUtils.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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;
+
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.util.PropertyUtil;
+
+import static
org.apache.iceberg.TableProperties.WRITE_METADATA_USE_RELATIVE_PATH;
+import static
org.apache.iceberg.TableProperties.WRITE_METADATA_USE_RELATIVE_PATH_DEFAULT;
+
+/**
+ * Utility class that contains path conversion methods.
+ */
+public final class MetadataPathUtils {
+
+ private MetadataPathUtils() {
+ }
+
+ /**
+ * Convert a given relative path to absolute path for the table by appending
the base table location
+ * @param path relative path to be converted
+ * @param tableLocation base table location
+ * @param shouldUseRelativePaths whether relative paths should be used
+ * @return the absolute path
+ */
+ public static String toAbsolutePath(String path, String tableLocation,
boolean shouldUseRelativePaths) {
+ Preconditions.checkArgument(path != null && path.trim().length() > 0);
+ // TODO: Fix this after tests are changed to always pass the table
location. Table location cannot be null.
+ if (tableLocation == null) {
+ return path;
+ }
+ // convert to absolute path by appending the table location
+ return shouldUseRelativePaths && !path.startsWith(tableLocation) ?
tableLocation + "/" + path : path;
Review comment:
do we need shouldUseRelativePaths here? Can't we just use the return
!path.startsWith(tableLocation) ? Paths.join(tableLocation, path) : path?
##########
File path: core/src/main/java/org/apache/iceberg/MetadataPathUtils.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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;
+
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.util.PropertyUtil;
+
+import static
org.apache.iceberg.TableProperties.WRITE_METADATA_USE_RELATIVE_PATH;
+import static
org.apache.iceberg.TableProperties.WRITE_METADATA_USE_RELATIVE_PATH_DEFAULT;
+
+/**
+ * Utility class that contains path conversion methods.
+ */
+public final class MetadataPathUtils {
+
+ private MetadataPathUtils() {
+ }
+
+ /**
+ * Convert a given relative path to absolute path for the table by appending
the base table location
+ * @param path relative path to be converted
+ * @param tableLocation base table location
+ * @param shouldUseRelativePaths whether relative paths should be used
+ * @return the absolute path
+ */
+ public static String toAbsolutePath(String path, String tableLocation,
boolean shouldUseRelativePaths) {
+ Preconditions.checkArgument(path != null && path.trim().length() > 0);
+ // TODO: Fix this after tests are changed to always pass the table
location. Table location cannot be null.
+ if (tableLocation == null) {
+ return path;
+ }
+ // convert to absolute path by appending the table location
+ return shouldUseRelativePaths && !path.startsWith(tableLocation) ?
tableLocation + "/" + path : path;
+ }
+
+ /**
+ * Convert a given absolute path to relative path with respect to base table
location
+ * @param path the absolute path
+ * @param tableLocation the base table location
+ * @return relative path with respect to the base table location
+ */
+ public static String toRelativePath(String path, String tableLocation,
boolean shouldUseRelativePaths) {
Review comment:
do we need shouldUseRelativePaths here? Shouldn't the function
toRelativePath always return a relative path? The caller would know whether it
needs to call toRelativePath or toAbsolutePath?
##########
File path: core/src/main/java/org/apache/iceberg/MetadataPathUtils.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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;
+
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.util.PropertyUtil;
+
+import static
org.apache.iceberg.TableProperties.WRITE_METADATA_USE_RELATIVE_PATH;
+import static
org.apache.iceberg.TableProperties.WRITE_METADATA_USE_RELATIVE_PATH_DEFAULT;
+
+/**
+ * Utility class that contains path conversion methods.
+ */
+public final class MetadataPathUtils {
+
+ private MetadataPathUtils() {
+ }
+
+ /**
+ * Convert a given relative path to absolute path for the table by appending
the base table location
+ * @param path relative path to be converted
+ * @param tableLocation base table location
+ * @param shouldUseRelativePaths whether relative paths should be used
+ * @return the absolute path
+ */
+ public static String toAbsolutePath(String path, String tableLocation,
boolean shouldUseRelativePaths) {
+ Preconditions.checkArgument(path != null && path.trim().length() > 0);
+ // TODO: Fix this after tests are changed to always pass the table
location. Table location cannot be null.
+ if (tableLocation == null) {
+ return path;
+ }
+ // convert to absolute path by appending the table location
+ return shouldUseRelativePaths && !path.startsWith(tableLocation) ?
tableLocation + "/" + path : path;
+ }
+
+ /**
+ * Convert a given absolute path to relative path with respect to base table
location
+ * @param path the absolute path
+ * @param tableLocation the base table location
+ * @return relative path with respect to the base table location
+ */
+ public static String toRelativePath(String path, String tableLocation,
boolean shouldUseRelativePaths) {
+ Preconditions.checkArgument(path != null && path.trim().length() > 0);
+ // TODO: Fix this after tests are changed to always pass the table
location. Table location cannot be null.
+ if (tableLocation == null) {
+ return path;
+ }
+ // convert to relative path by removing the table location
+ return shouldUseRelativePaths && path.startsWith(tableLocation) ?
Review comment:
I think for both of these functions we are basically care more about the
output than the input
toAbsolute
(Absolute Path | Relative to table Location) -> Absolute Path
toRelative
(Absolute Path | Relative to table location) -> relativizedPath to table
location | absolutePath not including table location
As @pvary notes it's possible for AbsolutePath to be in a completely
different file system, in that case this method sometimes returns relative
paths, but sometimes returns absolute paths.
I think this is actually the right thing to do for this method since we can
only ever deal with paths relative to the table location ... but maybe this
needs another name though? Since currently it doesn't always return relative
paths
##########
File path: api/src/main/java/org/apache/iceberg/Snapshot.java
##########
@@ -126,4 +127,10 @@
* @return the location of the manifest list for this Snapshot
*/
String manifestListLocation();
+
+ /**
+ *
+ * @return file io used for this snapshot
+ */
+ FileIO io();
Review comment:
Why do we need this?
##########
File path: core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java
##########
@@ -158,7 +158,8 @@ private ManifestFile copyManifest(ManifestFile manifest) {
InputFile toCopy = ops.io().newInputFile(manifest.path());
OutputFile newFile = newManifestOutput();
return ManifestFiles.copyRewriteManifest(
- current.formatVersion(), toCopy, specsById, newFile, snapshotId(),
summaryBuilder);
+ current.formatVersion(), toCopy, specsById, newFile, snapshotId(),
summaryBuilder,
+ current.location(), current.shouldUseRelativePaths());
Review comment:
shouldn't I shouldUseRelativePaths from the table properties?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
