SEPURI-SAI-KRISHNA commented on code in PR #19648:
URL: https://github.com/apache/hudi/pull/19648#discussion_r3836264173
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/keygen/PartitionPathFormatterBase.java:
##########
@@ -75,15 +76,11 @@ public final S combine(List<String> partitionPathFields,
Object... partitionPath
if (useHiveStylePartitioning) {
sb.appendJava(partitionPathFields.get(i))
- .appendJava("=")
- .append(partitionPathPartStr);
- } else if (slashSeparatedDatePartitioning) {
- String res = ((String) partitionPathPartStr).replace('-', '/');
- sb.append(((S) res));
- } else {
- sb.append(partitionPathPartStr);
+ .appendJava("=");
}
+ sb.append(partitionPathPartStr);
Review Comment:
Confirmed, and fixed by restoring the old behaviour rather than by teaching
the file index about key generators.
I traced both sides. `CustomKeyGenerator#getPartitionKeyGenerators` builds
one single-field sub-keygen per field and `getPartitionPath` appends each
result joined by `/`, so every field takes the single-part branch and the
writer lands in `2026/01/05/NYC`. `composeRelativePartitionPath` builds a
`StringPartitionPathFormatter` and calls `combine` once over all N columns, so
it lands in the multi-part branch. Before this PR that branch substituted every
part too -- via the `(String)` cast that is the `ClassCastException` -- so the
two agreed. Dropping the substitution there is what makes the prefix miss.
So the multi-part branch now substitutes every part again, but through
`replaceDashesWithSlashes` and the guard instead of the cast:
```java
} else if (slashSeparatedDatePartitioning &&
!hasPathBreakingDash(partitionPathPartStr)) {
sb.append(replaceDashesWithSlashes(partitionPathPartStr));
}
```
That keeps this PR to the `ClassCastException`/NPE fix it is named for, with
no behaviour change on the multi-field path, and leaves whether such a layout
should exist at all to #19666.
Test added as asked -- `Test slash separated date partitions with a
multi-field CustomKeyGenerator`: two columns, both bound in the predicate so
the read takes the all-bound short circuit at `:446-448`, asserting the row
comes back. With the substitution neutralized it fails exactly as you
described, silently:
```
Expected Array([1,a1]), but got Array()
```
Worth noting the short circuit builds the `PartitionPath` from the bound
values without parsing the directory, which is why this is reachable at all on
a multi-field slash table -- `doParsePartitionColumnValues` would not survive
the extra fragments.
This also corrects something I agreed to too quickly in the last round: I
accepted that the multi-part branch was unreachable. That holds for the write
paths, but not for the read side.
--
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]
