[WIP] Core: Support relative paths in metadata #2658

anuragmantri · 2021-06-01T17:48:06Z

Add initial support for relative paths in metadata(#1617).

Summary of changes:

Introduce write.metadata.use.relative-path property that governs the use of relative paths
Modify metadata writers and readers to use relative paths when the above property is enabled
No changes were made to public APIs

flyrain · 2021-06-01T22:18:22Z

core/src/main/java/org/apache/iceberg/TableProperties.java

@@ -177,6 +177,9 @@ private TableProperties() {
  public static final String UPDATE_ISOLATION_LEVEL = "write.update.isolation-level";
  public static final String UPDATE_ISOLATION_LEVEL_DEFAULT = "serializable";

+  public static final String WRITE_METADATA_USE_RELATIVE_PATH = "write.metadata.use.relative-path";


It is worth to comment out in the code that this property is immutable once the table is created.

anuragmantri · 2021-06-01T23:40:32Z

All tests in spark3/src/test/java/org/apache/iceberg/actions/TestRemoveOrphanFilesAction3.java are passing for me locally. However, there is a test failure (testHiveCatalogTable.java) in CI. Has this happed to anyone before?

pvary · 2021-06-09T09:20:24Z

core/src/main/java/org/apache/iceberg/DataFilesTable.java

@@ -122,7 +122,7 @@ public long targetSplitSize() {
    @Override
    public CloseableIterable<StructLike> rows() {
      return CloseableIterable.transform(
-          ManifestFiles.read(manifest, io).project(schema),
+          ManifestFiles.read(manifest, io, null, null).project(schema),


Could we keep the original signatures as well, so we do not have to rewrite the method calls everywhere and add null, null? I think this could greatly reduce the changes needed in this patch

pvary · 2021-06-09T09:21:47Z

core/src/main/java/org/apache/iceberg/DataTableScan.java

@@ -71,7 +71,8 @@ protected TableScan newRefinedScan(TableOperations ops, Table table, Schema sche
  public CloseableIterable<FileScanTask> planFiles(TableOperations ops, Snapshot snapshot,
                                                   Expression rowFilter, boolean ignoreResiduals,
                                                   boolean caseSensitive, boolean colStats) {
-    ManifestGroup manifestGroup = new ManifestGroup(ops.io(), snapshot.dataManifests(), snapshot.deleteManifests())
+    ManifestGroup manifestGroup = new ManifestGroup(ops.io(), snapshot.dataManifests(), snapshot.deleteManifests(),
+        ops.current().location(), ops.current().properties())


Why do we push the whole properties? Shall we just push a flag useRelative, or we can just push the location and if it is not null then we use it as a base for the relative paths?

Instead of pushing properties around, I introduced a flag shouldUseRelativePaths.

pvary · 2021-06-09T09:24:34Z

core/src/main/java/org/apache/iceberg/InheritableMetadataFactory.java

    }

    @Override
    public <F extends ContentFile<F>> ManifestEntry<F> apply(ManifestEntry<F> manifestEntry) {
      if (manifestEntry.file() instanceof BaseFile) {
        BaseFile<?> file = (BaseFile<?>) manifestEntry.file();
        file.setSpecId(specId);
+        if (MetadataPaths.useRelativePath(tableProperties)) {
+          if (!file.path().toString().startsWith(tableLocation)) {


Why do not we push this logic inside the otAbsolutePath method?

This is only a question which we might to consider, I myself not sure about the answer yet.

You are right. Pushed the logic into toAbsolutePath().

pvary · 2021-06-09T09:25:00Z

core/src/main/java/org/apache/iceberg/ManifestEntriesTable.java

@@ -126,10 +126,10 @@ public long targetSplitSize() {
    @Override
    public CloseableIterable<StructLike> rows() {
      if (manifest.content() == ManifestContent.DATA) {
-        return CloseableIterable.transform(ManifestFiles.read(manifest, io).project(fileSchema).entries(),
+        return CloseableIterable.transform(ManifestFiles.read(manifest, io, null, null).project(fileSchema).entries(),


Again, keep the original signatures as well

pvary · 2021-06-09T09:27:08Z

core/src/main/java/org/apache/iceberg/MetadataPaths.java

+/**
+ * Utility class that contains path conversion methods.
+ */
+public final class MetadataPaths {


Maybe name it as MetadataPathUtils?

pvary · 2021-06-09T09:31:03Z

core/src/main/java/org/apache/iceberg/MetadataPaths.java

+      return path;
+    }
+    // convert to absolute path by appending the table location
+    return useRelativePath(properties) && !path.startsWith(tableLocation) ? tableLocation + "/" + path : path;


For me extending the LocationProvider with additional default implementation which handles relative patch would be more natural than handling it in some random place in the code

IIUC, the LocationProvider class is for data files and is used to provide locations to write the files. In this PR, we do not wish to change where data files and/or manifest files are written. We just want to change the references to any paths inside metadata files to relative paths. This way, if a table is relocated/moved, we don't have to change a bunch of metadata files.

You are right. Would it be better to introduce something similar for metadata file locations as well? We can have 2 implementations:

AbsoluteMetadataLocationProvider

RelativeMetadataLocationProvider

Or even a single implementation at this point handling both?
This could hide the complexity of calculating the path whenever it is needed and we can just pass down this provider for every metadata file (even encapsulate the io, so we do not have to pass that around too)

pvary · 2021-06-09T09:33:54Z

spark2/src/test/java/org/apache/iceberg/examples/RelativePathHiveFunctionalityTest.java

+
+import static org.apache.iceberg.types.Types.NestedField.optional;
+
+public class RelativePathHiveFunctionalityTest extends HiveMetastoreTest {


If I am correct then we try to name test to start with Test, like TestRelativePathHiveFunctionality

You are right. Renamed the class.

pvary · 2021-06-09T09:40:40Z

@anuragmantri: Tried to go through the PR, but this one is huge 😄

Some general takeaways:

Can we keep the original signatures for the extended methods as well? They still can call the new ones, but if we kept the original ones then we do not have to change the callers to add null, null which could reduce the number of changes greatly
Can we decrease the number of parameters we push down the new methods? I would prefer to send a boolean useRelativePath instead of the whole property list, or even only send just the baseLocation if we have to use it.
Would it be possible to hide the whole stuff inside a LocationProvider? It could be a base implementation for all of the LocationProviders, and would be a natural place in my opinion. We can avoid changing the public APIs if we provide a default implementation.
If there is any way to split this PR to smaller, more reviewer friendly ones that would be a big help for us, reviewers.

Thanks,
Peter

anuragmantri · 2021-06-22T05:49:45Z

Thanks for the review @pvary :)

Would it be possible to hide the whole stuff inside a LocationProvider? It could be a base implementation for all of the LocationProviders, and would be a natural place in my opinion. We can avoid changing the public APIs if we provide a default implementation.

In this change, we only want to replace the references of paths in the content of metadata files. We do not wish to change where data/metadata is actually written. My understanding of LocationProviders is that they are used to get locations to write the data files and hence for this change, we don't want to change any LocationProvider implementation.

If there is any way to split this PR to smaller, more reviewer friendly ones that would be a big help for us, reviewers.

I tried to keep this change small initially. Specifically, in this PR, I tried to just introduce the new property and modify the read and write paths to use table property to do the necessary changes. Property is turned off my default. Yet, this turned out to be huge :) There will be several follow-up PRs such has handling metadata tables and writing unit tests. I will be happy to split this further if you think we can logically do so.

pvary · 2021-06-22T09:08:44Z

core/src/main/java/org/apache/iceberg/BaseSnapshot.java

@@ -180,14 +210,20 @@ public String manifestListLocation() {
    return manifestListLocation;
  }

+  @Override
+  public FileIO io() {


I leave a comment here, because I would rather not leak io here if possible

pvary · 2021-06-22T09:10:24Z

core/src/main/java/org/apache/iceberg/MetadataPathUtils.java

+      return path;
+    }
+    // convert to absolute path by appending the table location
+    return shouldUseRelativePaths && !path.startsWith(tableLocation) ? tableLocation + "/" + path : path;


I do not like concatenating "/" in path. Maybe using Path object would be better.

do we need shouldUseRelativePaths here? Can't we just use the return !path.startsWith(tableLocation) ? Paths.join(tableLocation, path) : path?

pvary · 2021-06-22T09:16:09Z

core/src/main/java/org/apache/iceberg/MetadataPathUtils.java

+   * @param properties table properties
+   * @return true if "write.metadata.use.relative-path" property is true, false otherwise
+   */
+  public static boolean shouldUseRelativePath(Map<String, String> properties) {


Maybe Table as a parameter as mentioned in the comments?

pvary · 2021-06-22T09:17:08Z

core/src/main/java/org/apache/iceberg/MetadataPathUtils.java

+      return path;
+    }
+    // convert to relative path by removing the table location
+    return shouldUseRelativePaths && path.startsWith(tableLocation) ?


If we should use relative paths, it might be an error if the path is not starts with location, and it is not relative...
Do we want that check here?

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

flyrain · 2021-06-22T17:40:59Z

Hi @aokolnychyi, @rdblue, can you take a look at this PR? Data replication is a pretty common request for any serious Iceberg user. Let's see how can we move forward with #1617. Thanks.

anuragmantri · 2021-06-25T19:01:05Z

CC: @dpaani - Who has also contributed to this work.

rdblue · 2021-06-25T23:04:56Z

@flyrain, thanks for pointing out this PR. Looks like the design doc was updated, so I'll make some time to take another look at that.

RussellSpitzer · 2021-06-28T17:54:47Z

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,


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"

RussellSpitzer · 2021-06-28T18:17:30Z

core/src/main/java/org/apache/iceberg/MetadataPathUtils.java

+   * @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) {


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?

RussellSpitzer · 2021-06-28T19:01:43Z

api/src/main/java/org/apache/iceberg/Snapshot.java

+   *
+   * @return file io used for this snapshot
+   */
+  FileIO io();


Why do we need this?

- Kept all the default signatures to prevent changes that are not necessary - Introduced a new flag shouldUseRelativePaths to determine if a table uses relative paths

github-actions · 2024-07-17T00:13:24Z

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.

github-actions · 2024-07-24T00:14:05Z

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.

anuragmantri changed the title ~~Support relative paths in metadata~~ Core: Support relative paths in metadata Jun 1, 2021

github-actions bot added API core flink spark labels Jun 1, 2021

flyrain reviewed Jun 1, 2021

View reviewed changes

anuragmantri force-pushed the support-relative-paths branch from f5f9855 to fd894b6 Compare June 1, 2021 22:52

pvary reviewed Jun 9, 2021

View reviewed changes

anuragmantri force-pushed the support-relative-paths branch from fd894b6 to 61f0907 Compare June 22, 2021 05:36

anuragmantri force-pushed the support-relative-paths branch 2 times, most recently from 75a55d7 to de261fd Compare June 22, 2021 07:11

pvary reviewed Jun 22, 2021

View reviewed changes

RussellSpitzer reviewed Jun 28, 2021

View reviewed changes

anuragmantri force-pushed the support-relative-paths branch from 30308fd to 0bb516e Compare October 19, 2021 22:33

github-actions bot added the MR label Oct 27, 2021

anuragmantri force-pushed the support-relative-paths branch 3 times, most recently from bedb035 to 02f650e Compare November 2, 2021 19:10

anuragmantri force-pushed the support-relative-paths branch from 02f650e to 92f402b Compare November 8, 2021 22:29

anuragmantri and others added 11 commits November 18, 2021 14:31

Core: Support relative paths in table metadata

27b9814

Addressed review comments

2a16de3

- Kept all the default signatures to prevent changes that are not necessary - Introduced a new flag shouldUseRelativePaths to determine if a table uses relative paths

Rebase with main

6105888

Add support for table location prefixes

8c6d3d7

Fix NPE in HadoopTableBuilder.withLocationPrefix()

204eda3

Null check in backward compatibility tests

7db42d4

Add unit tests in TestTableMetadata

c809af0

Fix tests

66f40ae

Fix relativization logic

156deab

Add unit tests

181af05

Internal: Register existing tables in Iceberg HiveCatalog

e3e9659

anuragmantri force-pushed the support-relative-paths branch from 92f402b to f710a8e Compare January 23, 2022 00:42

github-actions bot added the hive label Jan 23, 2022

Add TestRelativePaths.java

e5f02c5

anuragmantri force-pushed the support-relative-paths branch from f5798bc to e5f02c5 Compare March 17, 2022 04:27

anuragmantri changed the title ~~Core: Support relative paths in metadata~~ [WIP] Core: Support relative paths in metadata Mar 17, 2022

anuragmantri mentioned this pull request Mar 17, 2022

Make DeleteOrphanFiles in Spark reliable #4346

Closed

Add more tests

defb763

github-actions bot added the stale label Jul 17, 2024

github-actions bot closed this Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Core: Support relative paths in metadata #2658

[WIP] Core: Support relative paths in metadata #2658

anuragmantri commented Jun 1, 2021

flyrain Jun 1, 2021

anuragmantri Jun 1, 2021

anuragmantri commented Jun 1, 2021

pvary Jun 9, 2021

pvary Jun 9, 2021

anuragmantri Jun 22, 2021

pvary Jun 9, 2021

anuragmantri Jun 22, 2021

pvary Jun 9, 2021

anuragmantri Jun 22, 2021

pvary Jun 9, 2021

anuragmantri Jun 22, 2021

pvary Jun 9, 2021 •

edited

Loading

anuragmantri Jun 22, 2021

pvary Jun 22, 2021

pvary Jun 9, 2021

anuragmantri Jun 22, 2021

pvary commented Jun 9, 2021

anuragmantri commented Jun 22, 2021

pvary Jun 22, 2021

pvary Jun 22, 2021

RussellSpitzer Jun 28, 2021

RussellSpitzer Jun 28, 2021

pvary Jun 22, 2021

pvary Jun 22, 2021

RussellSpitzer Jun 28, 2021

flyrain commented Jun 22, 2021 •

edited

Loading

anuragmantri commented Jun 25, 2021 •

edited

Loading

rdblue commented Jun 25, 2021

RussellSpitzer Jun 28, 2021

RussellSpitzer Jun 28, 2021

RussellSpitzer Jun 28, 2021

github-actions bot commented Jul 17, 2024

github-actions bot commented Jul 24, 2024


		import static org.apache.iceberg.types.Types.NestedField.optional;

		public class RelativePathHiveFunctionalityTest extends HiveMetastoreTest {

[WIP] Core: Support relative paths in metadata #2658

[WIP] Core: Support relative paths in metadata #2658

Conversation

anuragmantri commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuragmantri commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvary Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvary commented Jun 9, 2021

anuragmantri commented Jun 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain commented Jun 22, 2021 • edited Loading

anuragmantri commented Jun 25, 2021 • edited Loading

rdblue commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 17, 2024

github-actions bot commented Jul 24, 2024

pvary Jun 9, 2021 •

edited

Loading

flyrain commented Jun 22, 2021 •

edited

Loading

anuragmantri commented Jun 25, 2021 •

edited

Loading