-
Notifications
You must be signed in to change notification settings - Fork 3k
Core: Fix relativize() to handle path equal to prefix (#15172) #15173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
szehon-ho
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, discussed offline with @Baunsgaard , I believe this was an initial oversight of the code
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java
Outdated
Show resolved
Hide resolved
| assertThat(RewriteTablePathUtil.relativize("/source/table/", "/source/table")).isEqualTo(""); | ||
|
|
||
| // Edge case: prefix has trailing separator | ||
| assertThat(RewriteTablePathUtil.relativize("/a", "/a/")).isEqualTo(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this pass? I guess /a cannot be relative to the given prefix of /a/, right? Unless we want to normalize file separator on both side
I think those 2 are ok as of now
assertThat(RewriteTablePathUtil.relativize("/a/", "/a")).isEqualTo("");
assertThat(RewriteTablePathUtil.relativize("/a/", "/a/")).isEqualTo("");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected that case to work. I added explicit handling for it, by modifying your suggested code to do the maybe append file separator :
if (!path.startsWith(toRemove) && !maybeAppendFileSeparator(path).equals(toRemove)) {
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java
Outdated
Show resolved
Hide resolved
364fc41 to
8da81fe
Compare
Fix RewriteTablePathUtil.relativize() throwing IllegalArgumentException when path equals prefix exactly, breaking rewrite_table_path when table properties like write.data.path point to the table root. The fix normalizes trailing separators for both path and prefix, treating '/a' and '/a/' as equivalent. This handles real-world cases where paths from different sources may have inconsistent trailing separators. Changes: - Fix relativize() to accept path equal to prefix - Normalize trailing separators on both path and prefix - Optimize combinePaths() for empty relative paths - Add comprehensive tests for all trailing separator combinations
8da81fe to
32a0d4a
Compare
|
Thanks for the review feedback @szehon-ho and @dramaticlly! I have updated the implementation, though with a slight variation from the suggested approach. What was suggestedif (!path.startsWith(toRemove) && !path.equals(prefix)) {
throw ...
}What I implementedif (!path.startsWith(toRemove) && !maybeAppendFileSeparator(path).equals(toRemove)) {
throw ...
}Why the differenceThe suggested approach using I chose to normalize both sides because:
This means all trailing separator combinations now work: |
What
Fix
RewriteTablePathUtil.relativize()to handle the edge case wherepathequalsprefixexactly.Why
Currently,
relativize()fails when a path equals the prefix (e.g.,write.data.pathset to the table root). This breaksrewrite_table_pathfor tables with properties pointing to the table location itself.Example failure:
// Throws IllegalArgumentException instead of returning ""
RewriteTablePathUtil.relativize("/path/to/table", "/path/to/table");
Use case:
Storage migration or replication where
write.data.path = table location.How
Added a check for exact match after normalizing trailing separators:
Changes
RewriteTablePathUtil.java: Fixrelativize()+ updated Javadoc forrelativize()andnewPath()TestRewriteTablePathUtil.java: Added 10 test methods covering:/tablevs/table-old)Testing
./gradlew :iceberg-core:test --tests "org.apache.iceberg.TestRewriteTablePathUtil"