dramaticlly commented on code in PR #15173:
URL: https://github.com/apache/iceberg/pull/15173#discussion_r2765718837
##########
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:
As you mentioned, I believe previously the `relativize` never returns empty,
so newPath internally call combinePaths will never returns trailing slash at
the end.
So I think we shall only apply maybeAppendFileSeparator to absolutePath when
relativePath is non-empty.
--
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]
