dramaticlly commented on code in PR #15173:
URL: https://github.com/apache/iceberg/pull/15173#discussion_r2760772040
##########
core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java:
##########
@@ -80,4 +81,157 @@ public void testStagingPathWithNoMiddlePart() {
RewriteTablePathUtil.stagingPath(fileDirectlyUnderPrefix,
sourcePrefix, stagingDir);
assertThat(newMethodResult).isEqualTo("/staging/file.parquet");
}
+
+ @Test
+ public void testRelativize() {
+ // Normal case: path is under prefix
+ assertThat(RewriteTablePathUtil.relativize("/a/b/c",
"/a")).isEqualTo("b/c");
+ assertThat(RewriteTablePathUtil.relativize("/a/b", "/a")).isEqualTo("b");
+
+ // Edge case: path equals prefix exactly (issue #15172)
+ assertThat(RewriteTablePathUtil.relativize("/a", "/a")).isEqualTo("");
+ assertThat(RewriteTablePathUtil.relativize("s3://bucket/warehouse",
"s3://bucket/warehouse"))
+ .isEqualTo("");
+
+ // Trailing separator variations - all combinations should work
+ assertThat(RewriteTablePathUtil.relativize("/a/", "/a")).isEqualTo("");
+ assertThat(RewriteTablePathUtil.relativize("/a/", "/a/")).isEqualTo("");
+ assertThat(RewriteTablePathUtil.relativize("/a", "/a/")).isEqualTo("");
+ }
+
+ @Test
+ public void testRelativizeInvalid() {
+ // Path does not start with prefix
+ assertThatThrownBy(() -> RewriteTablePathUtil.relativize("/other/path",
"/source/table"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("does not start with");
+
+ // Overlapping names: /table-old should NOT match prefix /table
+ assertThatThrownBy(() ->
RewriteTablePathUtil.relativize("/table-old/data", "/table"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("does not start with");
+ }
+
+ @Test
+ public void testNewPath() {
+ // Normal case: path is under prefix
+ assertThat(RewriteTablePathUtil.newPath("/src/data/file.parquet", "/src",
"/tgt"))
+ .isEqualTo("/tgt/data/file.parquet");
+
+ // Trailing separator on path
+ assertThat(RewriteTablePathUtil.newPath("/src/data/", "/src",
"/tgt")).isEqualTo("/tgt/data/");
+
+ // Both path and prefix with trailing separator
+ assertThat(RewriteTablePathUtil.newPath("/src/", "/src/",
"/tgt")).isEqualTo("/tgt/");
+ }
+
+ @Test
+ public void testNewPathEqualsPrefix() {
+ // Issue #15172: path equals prefix (e.g., write.data.path = table
location)
+ // Result has trailing separator since directories are represented with
trailing separators
+ assertThat(RewriteTablePathUtil.newPath("/src", "/src",
"/tgt")).isEqualTo("/tgt/");
Review Comment:
this seem to be a bit concerning, not sure if we want to have file separator
at the end as this newPath is used in many place, not necessary just for
directories.
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -754,14 +762,29 @@ public static String fileName(String path) {
return filename;
}
- /** Relativize a path. */
+ /**
+ * Compute the relative path from a prefix to a given path.
+ *
+ * <p>If the path is under the prefix, returns the portion after the prefix.
If the path equals
+ * the prefix (representing the root directory itself), returns an empty
string.
+ *
+ * <p>Trailing separators are normalized: "/a" and "/a/" are treated as
equivalent for both path
+ * and prefix. This allows flexibility when paths come from different
sources that may or may not
+ * include trailing separators.
+ *
+ * @param path absolute path to relativize
+ * @param prefix prefix path to remove
+ * @return relative path from prefix to path, or empty string if path equals
prefix
+ * @throws IllegalArgumentException if path is not under or equal to prefix
+ */
public static String relativize(String path, String prefix) {
String toRemove = maybeAppendFileSeparator(prefix);
- if (!path.startsWith(toRemove)) {
+ String normalizedPath = maybeAppendFileSeparator(path);
+ if (!normalizedPath.startsWith(toRemove)) {
throw new IllegalArgumentException(
String.format("Path %s does not start with %s", path, toRemove));
Review Comment:
nit, since now it checks for normalizedPath, I would update the exception
message to reflect normalizedPath instead of 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: [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]
