SEPURI-SAI-KRISHNA commented on code in PR #19648:
URL: https://github.com/apache/hudi/pull/19648#discussion_r3836313076
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/KeyGenUtils.java:
##########
@@ -291,12 +292,30 @@ public static String getPartitionPath(GenericRecord
record, String partitionPath
if (hiveStylePartitioning) {
partitionPath = partitionPathField + "=" + partitionPath;
}
+ // NOTE: See [[slashSeparateDateValue]] on why a leading dash suppresses
the substitution
if (slashSeparatedDatePartitioning) {
- partitionPath = partitionPath.replace('-', '/');
+ partitionPath = slashSeparateDateValue(partitionPath);
}
return partitionPath;
}
+ /**
+ * Turns a {@code yyyy-MM-dd} formatted date value into the {@code
yyyy/MM/dd} directory structure
+ * requested by {@code
hoodie.datasource.write.slash.separated.date.partitioning}.
+ *
+ * <p>A value with a leading dash is returned as-is: substituting would make
the partition path
+ * start with {@code "/"}, and an absolute relative-partition-path is
resolved inconsistently --
+ * {@link org.apache.hudi.common.fs.FSUtils#constructAbsolutePath(String,
String)} chops the
+ * leading {@code "/"} while the {@link org.apache.hudi.storage.StoragePath}
overload used by
+ * {@code AbstractTableFileSystemView} lets it URI-resolve away the table
base path (a value of
+ * {@code "-5"} lands the writer in {@code "<base>/5"} but the file-system
view in {@code "/5"},
+ * and {@code "-"} resolves to the base path itself). Such a value is not a
date to begin with,
+ * so nothing is lost by not slashing it.
+ */
+ private static String slashSeparateDateValue(String partitionPath) {
+ return partitionPath.startsWith("-") ? partitionPath :
partitionPath.replace('-', '/');
Review Comment:
Applied, both cases confirmed. The predicate is widened and now lives in one
place, `KeyGenUtils#hasPathBreakingDash`:
```java
public static boolean hasPathBreakingDash(String partitionPath) {
return partitionPath.startsWith("-") || partitionPath.endsWith("-") ||
partitionPath.contains("--");
}
```
`StringPartitionPathFormatter` delegates to it (which also answers the bot's
`startsWith("-")` nit on the same file) and `UTF8StringPartitionPathFormatter`
mirrors it with a `DOUBLE_DASH_UTF8` constant, since
`startsWith`/`endsWith`/`contains` all exist on `UTF8String`. `startsWithDash`
is gone from the base class and both subclasses.
The javadoc now records all three shapes and why each one breaks: the
leading case as before, the trailing and doubled cases because the recorded
partition string ends up longer than the directory it normalizes to, so
`FSUtils#getFileName` slices at the wrong offset.
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/keygen/TestPartitionPathFormatter.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.keygen;
+
+import org.apache.spark.unsafe.types.UTF8String;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.apache.hudi.keygen.KeyGenUtils.HUDI_DEFAULT_PARTITION_PATH;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Tests the partition-path formatters backing the key generators, making sure
that both the
+ * {@link String} (Avro/{@link org.apache.spark.sql.Row} write path) and the
{@link UTF8String}
+ * (row-writer/{@link org.apache.spark.sql.catalyst.InternalRow} write path)
flavors produce
+ * identical partition paths.
+ */
+class TestPartitionPathFormatter {
+
+ private static final List<String> SINGLE_FIELD =
Collections.singletonList("date_col");
+ private static final List<String> TWO_FIELDS = Arrays.asList("date_col",
"city");
+
+ private String combine(boolean useRowWriterPath,
+ boolean hiveStylePartitioning,
+ boolean encode,
+ boolean slashSeparatedDatePartitioning,
+ List<String> fields,
+ Object... parts) {
+ if (useRowWriterPath) {
+ return new UTF8StringPartitionPathFormatter(
+ UTF8StringPartitionPathFormatter.UTF8StringBuilder::new,
hiveStylePartitioning, encode,
+ slashSeparatedDatePartitioning).combine(fields, parts).toString();
+ }
+ return new StringPartitionPathFormatter(
+ StringPartitionPathFormatter.JavaStringBuilder::new,
hiveStylePartitioning, encode,
+ slashSeparatedDatePartitioning).combine(fields, parts);
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ void
testSlashSeparatedDatePartitioningOnlyAppliesToSingleFieldPartitioning(boolean
useRowWriterPath) {
+ // NOTE: This mirrors [[KeyGenUtils#getRecordPartitionPath]] driving the
Avro write-path
+ assertEquals("2026-01-05/san-francisco",
+ combine(useRowWriterPath, false, false, true, TWO_FIELDS,
"2026-01-05", "san-francisco"));
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ void testSlashSeparatedDatePartitioningHandlesNullAndEmptyValues(boolean
useRowWriterPath) {
+ assertEquals(HUDI_DEFAULT_PARTITION_PATH,
+ combine(useRowWriterPath, false, false, true, SINGLE_FIELD, new
Object[] {null}));
+ assertEquals(HUDI_DEFAULT_PARTITION_PATH,
+ combine(useRowWriterPath, false, false, true, SINGLE_FIELD, ""));
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ void testSlashSeparatedDatePartitioningLeavesLeadingDashesAlone(boolean
useRowWriterPath) {
+ // NOTE: Substituting here would yield a partition path starting with "/",
which
+ // [[FSUtils#constructAbsolutePath(String, String)]] and the
[[StoragePath]] overload used
+ // by [[AbstractTableFileSystemView]] resolve differently -- the
former chops the leading
+ // "/", the latter URI-resolves the table base path away -- so the
writer and the
+ // file-system view would disagree on where the partition lives
+ assertEquals("-5", combine(useRowWriterPath, false, false, true,
SINGLE_FIELD, "-5"));
+ assertEquals("-", combine(useRowWriterPath, false, false, true,
SINGLE_FIELD, "-"));
+ assertEquals("--5", combine(useRowWriterPath, false, false, true,
SINGLE_FIELD, "--5"));
+ // A dash anywhere else is still a separator: only a leading one produces
an absolute path
+ assertEquals("5/", combine(useRowWriterPath, false, false, true,
SINGLE_FIELD, "5-"));
Review Comment:
Applied. `5-` and `a--b` are asserted as returned unchanged, replacing the
`5/` line that pinned the broken shape, and the comment now states the real
invariant -- leading, trailing and doubled all fail the round trip, for the two
different reasons.
The single interior dash case stays as the positive control.
--
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]
