voonhous commented on code in PR #19648:
URL: https://github.com/apache/hudi/pull/19648#discussion_r3829072959
##########
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:
Right for `ComplexKeyGenerator`, but a regression for `CustomKeyGenerator`:
it builds one single-field sub-keygen per field (`CustomKeyGenerator.java:91`),
so every field is slashed on disk -- `2026/01/05/NYC`.
`composeRelativePartitionPath` calls `combine` once with all N columns, so
it lands here. It used to produce `2026/01/05/NYC` and the all-bound short
circuit at `SparkHoodieTableFileIndex.scala:447` returned rows; now it produces
`2026-01-05/NYC`, `exists()` fails at `:444`, and `:445` returns `Seq()`.
Silent zero rows, not the `HoodieException` of #19666. Untested:
`TestCustomKeyGenerator:419` is single-column.
**Ask:** add a two-column `CustomKeyGenerator` slash test with both columns
bound, asserting non-zero rows. Then either make `composeRelativePartitionPath`
keygen-aware, or fold this into #19666 and link it here.
##########
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:
A trailing dash and a doubled dash break the same round trip.
`StoragePath.normalize` (`StoragePath.java:300`) strips trailing slashes,
`URI.normalize()` collapses inner `//`:
```
5- -> 5/ writes <base>/5 lists back 5
a--b -> a//b writes <base>/a/b lists back a/b
```
The writer records the raw string (`HoodieRowCreateHandle.java:132,278`) but
resolves the directory normalized (`:303`). So `FSUtils.getFileName` slices at
`partition.length()+1` (`FSUtils.java:615`) and `FilesIndexer.java:185` stores
`xx.parquet` for `5/xxx.parquet` -- a corrupt metadata-table FILES entry.
Your compat argument holds unchanged: these outputs are already unreadable.
**Ask:** widen the predicate here,
```suggestion
return partitionPath.startsWith("-") || partitionPath.endsWith("-") ||
partitionPath.contains("--")
? partitionPath
: partitionPath.replace('-', '/');
```
and in both formatters -- rename `startsWithDash` and add `endsWith` /
`contains`. Both exist on `UTF8String` in spark-unsafe 3.3.4 and up.
##########
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:
This pins the broken shape, and the comment's invariant is false: `5-` gives
`5/` (writes `<base>/5`, records `5/`) and `a--b` gives `a//b`. Both fail the
round trip the guard exists to protect -- evidence in my comment on
`slashSeparateDateValue`. A green test here means the follow-up has to argue
with an assertion instead of a gap.
**Ask:** assert the guarded values. Goes green with the widened guard.
```suggestion
// A dash that would turn into a leading, trailing or doubled "/" is
left alone as well
assertEquals("5-", combine(useRowWriterPath, false, false, true,
SINGLE_FIELD, "5-"));
assertEquals("a--b", combine(useRowWriterPath, false, false, true,
SINGLE_FIELD, "a--b"));
```
##########
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);
Review Comment:
By `:293` the value is already `dt=-5`, so `startsWith("-")` can never fire
under hive-style: you get `dt=/5`, and `-` gives `dt=/`, whose directory is
`<base>/dt=`. The formatter guards the bare value instead
(`PartitionPathFormatterBase.java:68`), so the two write paths guard different
strings. `df.write` and HoodieStreamer still reach this --
`HoodieCatalogTable.scala:296-300` only blocks the SQL DDL path (#19669).
**Ask:** move `slashSeparateDateValue(...)` before the `field=` prefix, here
and at `:262`. Behaviour-preserving otherwise, since `"dt="` contains no dash.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/KeyGenUtils.java:
##########
@@ -257,8 +257,9 @@ public static String getRecordPartitionPath(GenericRecord
record,
if (hiveStylePartitioning) {
fieldVal = partitionPathField + "=" + fieldVal;
}
+ // NOTE: See [[slashSeparateDateValue]] on why a leading dash
suppresses the substitution
if (partitionPathFields.size() == 1 && slashSeparatedDatePartitioning)
{
- fieldVal = fieldVal.replace('-', '/');
+ fieldVal = slashSeparateDateValue(fieldVal);
Review Comment:
Untested. `SimpleAvroKeyGenerator.java:55` calls the other overload, so
`TestSimpleKeyGenerator#...LeavesLeadingDashesAlone` covers `:297`, not this
line. The only production caller here is `ComplexAvroKeyGenerator.java:53`, and
neither slash test that reaches it (`TestComplexKeyGenerator.java:228`, `:248`)
uses a leading-dash value.
**Ask:** add `testSlashSeparatedDatePartitioningLeavesLeadingDashesAlone`
next to `TestComplexKeyGenerator:228` -- same props, `put("timestamp", "-5")`,
assert `-5`. Three lines, and red without this guard.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/common/TestSlashSeparatedPartitionValue.scala:
##########
@@ -90,6 +90,113 @@ class TestSlashSeparatedPartitionValue extends
HoodieSparkSqlTestBase {
}
}
+ test("Test slash separated date partitions written through the row writer") {
+ withSQLConf("hoodie.spark.sql.insert.into.operation" -> "bulk_insert") {
+ withTempDir { tmp =>
+ val targetTable = generateTableName
+ val tablePath = s"${tmp.getCanonicalPath}/$targetTable"
+
+ spark.sql(
+ s"""
+ |create table $targetTable (
+ | `id` string,
+ | `name` string,
+ | `ts` bigint,
+ | `datestr` STRING
+ |) using hudi
+ | tblproperties (
+ | 'primaryKey' = 'id',
+ | 'type' = 'COW',
+ | 'preCombineField'='ts',
+ |
'hoodie.datasource.write.slash.separated.date.partitioning'='true'
+ | )
+ | partitioned by (`datestr`)
+ | location '$tablePath'
+ """.stripMargin)
+
+ // NOTE: The row writer derives the partition path off of an
[[InternalRow]], which used to
+ // blow up with a [[ClassCastException]]; a null partition value
used to NPE
+ spark.sql(
+ s"""
+ | insert into $targetTable values
+ | (1, 'a1', 1000, "2026-01-05"),
+ | (2, 'a2', 2000, "2026-01-06"),
+ | (3, 'a3', 3000, null)
+ """.stripMargin)
+
+ checkAnswer(s"select id, name, ts, _hoodie_partition_path, datestr
from $targetTable order by id")(
+ Seq("1", "a1", 1000, "2026/01/05", "2026-01-05"),
+ Seq("2", "a2", 2000, "2026/01/06", "2026-01-06"),
+ Seq("3", "a3", 3000, "__HIVE_DEFAULT_PARTITION__", null)
+ )
+
+ val metaClient = HoodieTableMetaClient.builder()
+
.setConf(HadoopFSUtils.getStorageConfWithCopy(spark.sparkContext.hadoopConfiguration))
+ .setBasePath(tablePath)
+ .build()
+ assertTrue(metaClient.getStorage.exists(new StoragePath(tablePath,
"2026/01/05")),
+ s"Partition path 2026/01/05 should exist")
+ assertTrue(metaClient.getStorage.exists(new StoragePath(tablePath,
"2026/01/06")),
+ s"Partition path 2026/01/06 should exist")
+ assertTrue(metaClient.getStorage.exists(new StoragePath(tablePath,
"__HIVE_DEFAULT_PARTITION__")),
+ s"Partition path __HIVE_DEFAULT_PARTITION__ should exist")
Review Comment:
Both new tests drop the assertion that checks writer-name against
metadata-table-name agreement, which is what this PR is about. The two
pre-existing tests make it (`:80-88` and `:257-262`:
`HoodieBackedTableMetadata.getAllPartitionPaths` contains `2026/01/05`).
`_hoodie_partition_path` plus `storage.exists` still pass when the two disagree.
**Ask:** copy the block from `:80-88` into both new tests, asserting
`2026/01/05`, `2026/01/06` and `__HIVE_DEFAULT_PARTITION__`.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/common/TestSlashSeparatedPartitionValue.scala:
##########
@@ -90,6 +90,113 @@ class TestSlashSeparatedPartitionValue extends
HoodieSparkSqlTestBase {
}
}
+ test("Test slash separated date partitions written through the row writer") {
Review Comment:
Nothing in the repo upserts into a slash-partitioned table -- all four tests
here are COW insert-only, and no slash test uses MOR (`git grep -n
"slash.separated.date.partitioning" -- '*/test/*'`). That leaves index and
file-system-view agreement on a slash partition path unexercised, which is
where a mismatched partition string surfaces as duplicate records rather than
an exception.
**Ask:** add one upsert case -- insert into `2026/01/05`, upsert the same
key and partition value, assert the partition still holds one row and no second
directory appeared.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/common/TestSlashSeparatedPartitionValue.scala:
##########
@@ -90,6 +90,113 @@ class TestSlashSeparatedPartitionValue extends
HoodieSparkSqlTestBase {
}
}
+ test("Test slash separated date partitions written through the row writer") {
+ withSQLConf("hoodie.spark.sql.insert.into.operation" -> "bulk_insert") {
+ withTempDir { tmp =>
+ val targetTable = generateTableName
+ val tablePath = s"${tmp.getCanonicalPath}/$targetTable"
+
+ spark.sql(
+ s"""
+ |create table $targetTable (
+ | `id` string,
+ | `name` string,
+ | `ts` bigint,
+ | `datestr` STRING
+ |) using hudi
+ | tblproperties (
+ | 'primaryKey' = 'id',
+ | 'type' = 'COW',
+ | 'preCombineField'='ts',
+ |
'hoodie.datasource.write.slash.separated.date.partitioning'='true'
+ | )
+ | partitioned by (`datestr`)
+ | location '$tablePath'
+ """.stripMargin)
+
+ // NOTE: The row writer derives the partition path off of an
[[InternalRow]], which used to
+ // blow up with a [[ClassCastException]]; a null partition value
used to NPE
+ spark.sql(
+ s"""
+ | insert into $targetTable values
+ | (1, 'a1', 1000, "2026-01-05"),
+ | (2, 'a2', 2000, "2026-01-06"),
+ | (3, 'a3', 3000, null)
+ """.stripMargin)
+
+ checkAnswer(s"select id, name, ts, _hoodie_partition_path, datestr
from $targetTable order by id")(
+ Seq("1", "a1", 1000, "2026/01/05", "2026-01-05"),
+ Seq("2", "a2", 2000, "2026/01/06", "2026-01-06"),
+ Seq("3", "a3", 3000, "__HIVE_DEFAULT_PARTITION__", null)
+ )
+
+ val metaClient = HoodieTableMetaClient.builder()
+
.setConf(HadoopFSUtils.getStorageConfWithCopy(spark.sparkContext.hadoopConfiguration))
+ .setBasePath(tablePath)
+ .build()
+ assertTrue(metaClient.getStorage.exists(new StoragePath(tablePath,
"2026/01/05")),
+ s"Partition path 2026/01/05 should exist")
+ assertTrue(metaClient.getStorage.exists(new StoragePath(tablePath,
"2026/01/06")),
+ s"Partition path 2026/01/06 should exist")
+ assertTrue(metaClient.getStorage.exists(new StoragePath(tablePath,
"__HIVE_DEFAULT_PARTITION__")),
+ s"Partition path __HIVE_DEFAULT_PARTITION__ should exist")
+ }
+ }
+ }
+
+ test("Test slash separated date partitions on a DATE typed partition
column") {
+ Seq("insert", "bulk_insert").foreach { operation =>
Review Comment:
The `insert` leg passes on master unchanged:
`TestTypedPartitionValues.scala:28` (from #19652, an ancestor of this branch)
already covers DATE plus slash on the default `insert` operation, asserts the
same things, and adds partition pruning. Only `bulk_insert` discriminates the
row-writer rendering this PR fixes.
**Ask:** keep only the leg that can fail.
```suggestion
Seq("bulk_insert").foreach { operation =>
```
Better still, drop the loop and pull in the pruning assertions from
`TestTypedPartitionValues.scala:63-68`.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/common/TestSlashSeparatedPartitionValue.scala:
##########
@@ -90,6 +90,113 @@ class TestSlashSeparatedPartitionValue extends
HoodieSparkSqlTestBase {
}
}
+ test("Test slash separated date partitions written through the row writer") {
+ withSQLConf("hoodie.spark.sql.insert.into.operation" -> "bulk_insert") {
+ withTempDir { tmp =>
+ val targetTable = generateTableName
+ val tablePath = s"${tmp.getCanonicalPath}/$targetTable"
+
+ spark.sql(
+ s"""
+ |create table $targetTable (
+ | `id` string,
+ | `name` string,
+ | `ts` bigint,
+ | `datestr` STRING
+ |) using hudi
+ | tblproperties (
+ | 'primaryKey' = 'id',
+ | 'type' = 'COW',
+ | 'preCombineField'='ts',
+ |
'hoodie.datasource.write.slash.separated.date.partitioning'='true'
+ | )
+ | partitioned by (`datestr`)
+ | location '$tablePath'
+ """.stripMargin)
+
+ // NOTE: The row writer derives the partition path off of an
[[InternalRow]], which used to
+ // blow up with a [[ClassCastException]]; a null partition value
used to NPE
+ spark.sql(
+ s"""
+ | insert into $targetTable values
+ | (1, 'a1', 1000, "2026-01-05"),
+ | (2, 'a2', 2000, "2026-01-06"),
+ | (3, 'a3', 3000, null)
+ """.stripMargin)
+
+ checkAnswer(s"select id, name, ts, _hoodie_partition_path, datestr
from $targetTable order by id")(
+ Seq("1", "a1", 1000, "2026/01/05", "2026-01-05"),
+ Seq("2", "a2", 2000, "2026/01/06", "2026-01-06"),
+ Seq("3", "a3", 3000, "__HIVE_DEFAULT_PARTITION__", null)
+ )
+
+ val metaClient = HoodieTableMetaClient.builder()
+
.setConf(HadoopFSUtils.getStorageConfWithCopy(spark.sparkContext.hadoopConfiguration))
+ .setBasePath(tablePath)
+ .build()
+ assertTrue(metaClient.getStorage.exists(new StoragePath(tablePath,
"2026/01/05")),
+ s"Partition path 2026/01/05 should exist")
+ assertTrue(metaClient.getStorage.exists(new StoragePath(tablePath,
"2026/01/06")),
+ s"Partition path 2026/01/06 should exist")
+ assertTrue(metaClient.getStorage.exists(new StoragePath(tablePath,
"__HIVE_DEFAULT_PARTITION__")),
+ s"Partition path __HIVE_DEFAULT_PARTITION__ should exist")
+ }
+ }
+ }
+
+ test("Test slash separated date partitions on a DATE typed partition
column") {
+ Seq("insert", "bulk_insert").foreach { operation =>
+ withSQLConf("hoodie.spark.sql.insert.into.operation" -> operation) {
+ withTempDir { tmp =>
+ val targetTable = generateTableName
+ val tablePath = s"${tmp.getCanonicalPath}/$targetTable"
+
+ spark.sql(
Review Comment:
nit, feel free to ignore: fourth copy of this DDL block in the file, two of
them added here. Post-PR there are 4 x 17 lines of `create table` (`:36`,
`:99`, `:154`, `:206`), 4 x 4 lines of `metaClient.builder()`, and 9 `exists`
asserts -- 102 lines that vary only by partition column type and property value.
**Ask:** extract `createSlashPartitionedTable`, `buildMetaClient` and
`assertPartitionDirsExist`. About 31 lines of helper for 102 of copy-paste, and
the next slash test costs three lines instead of forty.
--
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]
