SEPURI-SAI-KRISHNA commented on code in PR #19648:
URL: https://github.com/apache/hudi/pull/19648#discussion_r3820480113
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/keygen/PartitionPathFormatterBase.java:
##########
@@ -62,11 +62,17 @@ public final S combine(List<String> partitionPathFields,
Object... partitionPath
// Avoid creating [[StringBuilder]] in case there's just one
partition-path part,
// and Hive-style of partitioning is not required
if (!useHiveStylePartitioning && partitionPathParts.length == 1) {
- if (slashSeparatedDatePartitioning) {
- return ((S) ((String) toString(partitionPathParts[0])).replace('-',
'/'));
- } else {
- return tryEncode(handleEmpty(toString(partitionPathParts[0])));
- }
+ S partitionPathPart =
tryEncode(handleEmpty(toString(partitionPathParts[0])));
+ // NOTE: For [[SimpleKeyGenerator]]/[[ComplexKeyGenerator]]
slash-separated date partitioning
+ // only kicks in for a table partitioned by a single (date)
column, mirroring
+ // [[KeyGenUtils#getPartitionPath]] (single field) and
[[KeyGenUtils#getRecordPartitionPath]]
+ // (which guards on a single field as well) driving the Avro
write-path: both write-paths
+ // have to derive the very same partition path for a record.
+ // [[CustomKeyGenerator]] is not an exception to this: it builds
one single-field
+ // sub-key-generator per partition field, so every field takes
this branch and a
+ // multi-field table does get each of its values slash-separated
-- on the Avro,
+ // [[org.apache.spark.sql.Row]] and
[[org.apache.spark.sql.catalyst.InternalRow]] paths alike
+ return slashSeparatedDatePartitioning ?
replaceDashesWithSlashes(partitionPathPart) : partitionPathPart;
Review Comment:
Confirmed and fixed in this PR. I probed the resolution directly rather than
reading it off, and it's slightly worse than described — the base path isn't
just chopped, it's replaced:
```
value partitionPath FSUtils(String, String) FSUtils(StoragePath,
String)
"-5" "/5" /tmp/mytable/5 /5
"--5" "//5" /5 (empty)
"-" "/" /tmp/mytable /tmp/mytable
"5-" "5/" /tmp/mytable/5 /tmp/mytable/5
```
So `-5` puts the writer in `<base>/5` and the file-system view in `/5`, `-`
collapses the partition onto the table root, and `--5` resolves to nothing at
all. Only a *leading* dash does this; an interior one (`a--b` → `a//b`)
URI-normalizes back to `a/b` harmlessly, and a trailing one is fine. Encoding
doesn't help — `escapePathName` leaves dashes alone.
Fixed by suppressing the substitution when the value starts with a dash, in
all three write paths:
* `PartitionPathFormatterBase#combine`, via a new `startsWithDash` abstract
implemented by both formatters, so the rule lives in one place rather than
being duplicated per string representation
* `KeyGenUtils#getPartitionPath` and `#getRecordPartitionPath`, via a shared
private `slashSeparateDateValue` helper
I went with the leading-dash guard rather than restricting the substitution
to `yyyy-MM-dd`. Matching the date format is closer to the config's intent, but
it silently changes the layout for anyone currently partitioning on a dashed
non-date value — `2026-01` writes `2026/01` today and would start writing
`2026-01` — which orphans existing partitions on a path that has shipped for
several releases. The leading-dash guard only changes values whose current
output is already broken and unreadable, so nothing that works today moves.
Happy to switch to the stricter check if you'd rather take that compat hit now.
Tests:
`TestPartitionPathFormatter#testSlashSeparatedDatePartitioningLeavesLeadingDashesAlone`
pins `-5`, `-`, `--5` alongside `5-` and `2026-01-05` across both formatters,
and
`TestSimpleKeyGenerator#testSlashSeparatedDatePartitioningLeavesLeadingDashesAlone`
covers the Avro path. Both fail without their respective guard (`expected:
<-5> but was: </5>`).
--
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]
