danny0405 commented on code in PR #19342:
URL: https://github.com/apache/hudi/pull/19342#discussion_r3717484480
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/CustomAvroKeyGenerator.java:
##########
@@ -161,7 +162,7 @@ public String getPartitionPath(GenericRecord record) {
partitionPath.append(DEFAULT_PARTITION_PATH_SEPARATOR);
}
}
- return partitionPath.toString();
+ return
PartitionPathEncodeUtils.validateNoPathTraversal(partitionPath.toString());
Review Comment:
Apache Hive and Iceberg do not explicitly encode `..`. They avoid traversal
primarily through structured partition-path construction.
- Hive escapes each column/value separately, then creates trusted separators
itself: `column=value/column=value`. Its escape list includes `/` and `\`, but
not `.`. Therefore a value containing `../` cannot create separators, while
`value=".."` becomes `column=..`, which is not a special filesystem segment.
[Hive
`FileUtils.makePartName`](https://github.com/apache/hive/blob/master/common/src/java/org/apache/hadoop/hive/common/FileUtils.java#L151-L173),
[[escape
list](https://github.com/apache/hive/blob/master/common/src/java/org/apache/hadoop/hive/common/FileUtils.java#L228-L247)](https://github.com/apache/hive/blob/master/common/src/java/org/apache/hadoop/hive/common/FileUtils.java#L228-L247)
- Iceberg follows the same structural approach. It URL-encodes the field and
value independently, then generates `field=value` components and inserts `/`
itself. [Iceberg
`PartitionSpec.partitionToPath`](https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L192-L208)
- Iceberg’s `URLEncoder` does not encode `.`, so its protection is the
`field=` prefix—not dot encoding. Java explicitly leaves `.` unchanged. [Java
`URLEncoder`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/net/URLEncoder.html)
For Hudi, the equivalent robust approach would separate full-path
construction from segment escaping:
```java
public static String escapePartitionPath(String path) {
return Arrays.stream(path.split("/", -1))
.map(PartitionPathEncodeUtils::escapePathSegment)
.collect(Collectors.joining("/"));
}
private static String escapePathSegment(String segment) {
// "." and ".." have filesystem semantics only when they occupy
// an entire path segment.
if (".".equals(segment) || "..".equals(segment)) {
return segment.replace(".", "%2E");
}
return doEscape(segment, PartitionPathEncodeUtils::needsEscaping);
}
```
Examples:
```java
escapePartitionPath("a/b/../c/d")
// a/b/%2E%2E/c/d
escapePartitionPath("../a/./b")
// %2E%2E/a/%2E/b
escapePartitionPath("a..b/v1.2")
// a..b/v1.2
```
Required imports:
```java
import java.util.Arrays;
import java.util.stream.Collectors;
```
This mirrors Hive/Iceberg’s key principle: split the path into components
first, escape each component, and introduce directory separators only in
trusted code.
Also, the existing Hudi `escapePathName("a/b/../c/d")` already produces
approximately:
```text
a%2Fb%2F..%2Fc%2Fd
```
That is safe because no literal `/` remains, so `..` is not a filesystem
segment. The new `escapePartitionPath` is needed only when Hudi intentionally
wants to preserve the hierarchical separators.
I did some investigation on Hive and Iceberg, we could auto escape the path
with similar way.
--
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]
