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:
Dropping the substitution from this branch is right for
`ComplexKeyGenerator`, but for `CustomKeyGenerator` it is a regression that
turns a working query into a silent empty result.
`CustomKeyGenerator` builds one single-field `SimpleKeyGenerator` per
partition field (`CustomKeyGenerator.java:91`, assembled at `:130-149`; Avro
mirror at `CustomAvroKeyGenerator.java:156-164`), so every field takes the
single-field fast path above and *is* slash-separated on disk. A
`dt:simple,city:simple` table lands in `2026/01/05/NYC` on the Avro, Row and
InternalRow paths alike -- your own javadoc on `replaceDashesWithSlashes` says
exactly this.
The query side does not decompose that way.
`SparkHoodieTableFileIndex.composeRelativePartitionPath` (`:471-489`) calls
`combine` once with all N partition columns, so it lands here in the
multi-field branch:
* before this PR it produced `2026/01/05/NYC`, `exists()` at
`SparkHoodieTableFileIndex.scala:444` passed, and the all-columns-bound short
circuit at `:447-448` returned the partition directly, without ever parsing the
path;
* after this PR it produces `2026-01-05/NYC`, `exists()` is false, and
`:445` returns `Seq()`.
So `select ... where dt = '2026-01-05' and city = 'NYC'` on a
`CustomKeyGenerator` slash table goes from returning rows to returning nothing,
with no error. This is distinct from #19666: that one is about the listing
path, whereas this short circuit never parses the partition path and therefore
worked end to end before this change. Nothing covers it --
`TestCustomKeyGenerator:419-448` is single-column, and every table in
`TestSlashSeparatedPartitionValue.scala` is `partitioned by (datestr)`.
One `combine` call cannot serve both generators: `KeyGenUtils.java:261`
guards the Avro multi-field path on `size() == 1`, while `CustomKeyGenerator`
decomposes into single-field sub-generators that each slash.
**Ask:** add a regression test that writes a two-column `CustomKeyGenerator`
slash table and reads it back with both partition columns bound in equality
predicates, asserting a non-zero row count. Then either make
`composeRelativePartitionPath` key-generator aware (one `combine` per column,
joined, when the table's key generator is `CustomKeyGenerator`), or, if you
would rather let #19666 reject the combination outright, say so in the PR
description and link it here. Silently converting rows into an empty result is
worse than the hard `HoodieException` #19666 describes.
##########
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:
The leading-dash guard covers one of three value shapes that produce a
partition path nothing agrees on. A trailing dash and a doubled dash are
equally broken, and the damage is worse than the leading-dash case.
The invariant that matters is that the recorded partition-path string
survives `FSUtils.getRelativePartitionPath(base, constructAbsolutePath(base,
p))`. `StoragePath.normalize` (`StoragePath.java:300`) strips trailing slashes
and `java.net.URI.normalize()` collapses an inner `//`:
```
value slashed directory listed back round-trips
2026-01-05 2026/01/05 <base>/2026/01/05 2026/01/05 yes
-5 -5 (guarded) <base>/-5 -5 yes
5- 5/ <base>/5 5 NO
a--b a//b <base>/a/b a/b NO
2026-01-05- 2026/01/05/ <base>/2026/01/05 2026/01/05 NO (collides
with 2026-01-05)
```
The writer resolves the directory with normalization but records the raw
string: `HoodieRowCreateHandle.java:303` builds the path via `new
StoragePath(basePath, partitionPath)`, while `:132` and `:278` store `5/` into
`WriteStatus` and `HoodieWriteStat.partitionPath`. Two concrete consequences:
1. `FilesIndexer.java:185` calls `FSUtils.getFileName(stat.getPath(),
partitionStatName)`, which slices at `partition.length() + 1`
(`FSUtils.java:615-618`). With partition `5/` and path `5/xxx.parquet` that
returns `xx.parquet` -- the metadata-table FILES record stores a filename
missing its first character.
2. On the read side `"5/".split("/")` yields a single fragment, so
`HoodieSparkUtils.doParsePartitionColumnValues` takes the equal-length branch
at `:344` and never applies `replace('/', '-')`; the value reads back as `5`,
not `5-`.
Your own compatibility argument for preferring this guard over a
`yyyy-MM-dd` match applies unchanged: these are values whose current output is
already unreadable, so no layout that works today moves.
**Ask:** widen the predicate in all three write paths.
```suggestion
return partitionPath.startsWith("-") || partitionPath.endsWith("-") ||
partitionPath.contains("--")
? partitionPath
: partitionPath.replace('-', '/');
```
and correspondingly in the formatter: rename `startsWithDash` to something
like `hasUnslashableDash` and implement it as `startsWith(DASH) ||
endsWith(DASH) || contains(DOUBLE_DASH)` in both
`StringPartitionPathFormatter:65` and `UTF8StringPartitionPathFormatter:71`.
`UTF8String.endsWith` and `UTF8String.contains` are present in spark-unsafe
3.3.4 through 3.5.5 (I checked the bytecode), so no version gate is needed. The
javadoc above and the one on `PartitionPathFormatterBase:125-136` need the same
widening.
##########
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:
The guard runs after the hive-style prefix has been prepended, so
`startsWith("-")` can never fire when `hiveStylePartitioning` is on: by `:293`
the value has already become `dt=-5`, and `slashSeparateDateValue` returns
`dt=/5`. The formatter checks the bare value instead
(`PartitionPathFormatterBase.java:68`, before any prefix is added), so the two
write paths apply the same guard to different strings.
`dt=/5` is a two-level directory for a single-column table, and a value of
`-` gives `dt=/`, whose directory is `<base>/dt=` -- so the recorded string
stops round-tripping to its own directory, which is the same failure
`FSUtils.getFileName` mis-slices on. `HoodieCatalogTable.scala:296-300` blocks
hive-style plus slash on the SQL DDL path only, so `df.write.format("hudi")`
and HoodieStreamer still reach this (the gap you filed as #19669).
**Ask:** move `slashSeparateDateValue(...)` so it runs on the bare field
value before the `field=` prefix is prepended, here and at `:262`. It is
behaviour-preserving for every value the guard does not catch, since `"dt=" +
"2026-01-05".replace('-','/')` equals `("dt=2026-01-05").replace('-','/')`, and
it makes the guard actually fire under hive-style partitioning.
##########
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:
This new guard has no test. `SimpleAvroKeyGenerator.java:55` calls the
*other* overload, so
`TestSimpleKeyGenerator#testSlashSeparatedDatePartitioningLeavesLeadingDashesAlone`
exercises `:297`, not this line. The only production caller of
`getRecordPartitionPath` is `ComplexAvroKeyGenerator.java:53`, and neither
slash test that reaches it uses a leading-dash value:
```
git grep -n "getRecordPartitionPath" -- '*.java' | grep -v /test/
grep -n "slash\|Slash"
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/keygen/TestComplexKeyGenerator.java
```
That leaves half of the new `KeyGenUtils` guard shipping unexercised.
**Ask:** add `testSlashSeparatedDatePartitioningLeavesLeadingDashesAlone` to
`TestComplexKeyGenerator` next to the existing `:228` -- same props,
`avroRecord.put("timestamp", "-5")`, assert `key.getPartitionPath()` equals
`-5`. It is three lines off the existing test and goes red without this guard
(`/5`), so it is a real regression guard rather than another characterization
test.
##########
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 as intended behaviour, and the comment states an
invariant that does not hold. A trailing dash produces a trailing-slash path
and `a--b` produces a doubled-slash path; both fail the same round trip the
leading-dash guard exists to protect. `5-` writes to `<base>/5` but records
`5/`, and `a--b` writes to `<base>/a/b` but records `a//b` -- evidence in my
comment on `KeyGenUtils#slashSeparateDateValue`. Once this merges, a green test
locks the wrong behaviour in and the follow-up has to argue with an assertion
rather than with a gap.
**Ask:** assert the guarded values instead, and widen the comment to name
all three bad shapes. This goes green once the guard change on
`slashSeparateDateValue` and the two formatters is in.
```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-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 one assertion in this file that checks writer-name
against metadata-table-name agreement, which is the invariant this whole PR is
about. The two pre-existing tests do make it: `:80-88` builds a
`HoodieBackedTableMetadata` and asserts `getAllPartitionPaths` contains
`2026/01/05`, and the test at `:257-262` does the same.
`_hoodie_partition_path` plus `storage.exists(...)` are not a substitute --
both still pass when the writer records a partition name that the metadata
table and directory listing disagree with (see my comment on
`KeyGenUtils#slashSeparateDateValue` for values where they do).
**Ask:** copy the `HoodieBackedTableMetadata` block from `:80-88` into both
new tests and assert `getAllPartitionPaths` contains `2026/01/05`, `2026/01/06`
and, for this one, `__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") {
+ 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 here duplicates an already-merged test, and is the weaker
of the two. `TestTypedPartitionValues.scala:28` ("Test reading a date partition
column laid out as yyyy/MM/dd", added in #19652, which is an ancestor of this
branch) already creates a DATE partition column with
`slash.separated.date.partitioning = 'true'`, writes with the default operation
-- `SPARK_SQL_INSERT_INTO_OPERATION` defaults to `insert` -- and asserts both
`_hoodie_partition_path` and the DATE round trip, plus two partition-pruning
assertions this test does not make. So the `insert` iteration passes on master
unchanged, and only the `bulk_insert` iteration 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 entirely and pull in the two pruning assertions
from `TestTypedPartitionValues.scala:63-68`, so the new test covers the
row-writer DATE path *and* pruning rather than re-running the Avro leg.
##########
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:
Cleanliness, not urgent: this is the fourth copy of the same DDL block in
this file, and this PR added two of the four. Exact counts in the post-PR file
(268 lines):
* `create table ... tblproperties(... slash.separated.date.partitioning ...)
partitioned by ... location`: four 17-line copies at `:36-52`, `:99-115`,
`:154-170`, `:206-222`, so 68 lines. Only two things vary -- the partition
column type (`STRING` three times, `DATE` once) and the property value.
*
`HoodieTableMetaClient.builder().setConf(...).setBasePath(tablePath).build()`:
four 4-line copies at `:68-71`, `:133-136`, `:187-190`, `:243-246`, so 16 lines.
* `assertTrue(metaClient.getStorage.exists(new StoragePath(tablePath,
...)))`: nine asserts over 18 lines.
**Ask:** extract three private helpers -- `createSlashPartitionedTable(name,
path, partitionColType = "STRING", slashSeparated = "true")`,
`buildMetaClient(path)` and `assertPartitionDirsExist(metaClient, path,
partitions*)`. That is roughly 31 lines of helper replacing 102 lines of
copy-paste, takes the file to about 209 lines, and makes the next slash test
cost three lines instead of forty.
##########
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:
Coverage gap worth closing while you are in this file: nothing in the repo
upserts into a slash-partitioned table. All four tests here are `type = 'COW'`
and insert-only, `TestTypedPartitionValues.scala` is insert-only, and no slash
test uses MOR:
```
git grep -n "slash.separated.date.partitioning\|SLASH_SEPARATED_DATE" --
'*/test/*'
```
That leaves index lookup and file-system-view agreement on a slash partition
path entirely unexercised, which is exactly the surface where a partition-path
string that does not match what listing produces shows up as duplicate records
rather than as an exception.
**Ask:** add one upsert case to this suite -- insert into `2026/01/05`, then
upsert a row with the same key and the same partition value, and assert the row
count in that partition is still one and that no second partition directory
appeared. A MOR variant would be a bonus, not a requirement.
--
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]
