voonhous commented on code in PR #19648:
URL: https://github.com/apache/hudi/pull/19648#discussion_r3819032311
##########
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:
The substitution is unconditional, so any value with a leading dash yields a
leading-slash path: `-5` becomes `/5`. `FSUtils.constructAbsolutePath(String,
String)` chops the leading `/` (`FSUtils.java:590-594`), but the `(StoragePath,
String)` overload used by `AbstractTableFileSystemView` does not, and
`StoragePath(parent, child)` URI-resolves an absolute child into replacing the
base path, so the writer and the file-system view disagree on where that
partition lives. Pre-existing on the Avro path (`KeyGenUtils` does the same
`replace`), but this PR makes it reachable on the row-writer paths.
Ask: fix this in this PR rather than a follow-up -- guard the substitution
(e.g. skip it when the value starts with a dash, or apply it only to values
matching `yyyy-MM-dd`) in `PartitionPathFormatterBase#combine` and in
`KeyGenUtils#getPartitionPath`/`getRecordPartitionPath` alike so all write
paths stay aligned, and add a `-5` assert to `TestPartitionPathFormatter`
pinning the guarded behavior.
--
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]
