fix(spark): make slash-separated date partitioning work on the row writer path - #19648
Conversation
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR fixes slash-separated date partitioning on the Spark row-writer (InternalRow/UTF8String) path by replacing the hard (String) cast with an abstract replaceDashesWithSlashes, and now routes single-field values through handleEmpty/tryEncode so nulls land in __HIVE_DEFAULT_PARTITION__ and encoding is preserved. The core logic and the new tests look sound; I traced the encoding-before-slash ordering and the multi-field behavior and they line up with the Avro path. One cross-path consistency corner (hive-style + slash) is worth double-checking in the inline comment. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. One small naming nit in the new test helper; otherwise the refactoring is clean and well-commented.
|
CI: the only failure is |
|
Thanks — both addressed. Renamed the test parameter On hive-style + slash-separated: the facts are right, but that combination is rejected at table creation, so it is unreachable rather than divergent — I also would not want to mirror the Avro behavior here: it applies Left the behavior as-is and added a comment on |
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR fixes slash-separated date partitioning on the Spark row-writer path by making the replaceDashesWithSlashes substitution operate on the concrete string representation (String/UTF8String) rather than hard-casting, and aligns the row-writer path with the Avro path by only slashing single-field partitions and routing through handleEmpty/tryEncode for null/empty and encoding. I traced the single-field and multi-field branches against KeyGenUtils.getRecordPartitionPath/getPartitionPath and the encode/slash ordering and null handling line up. The one remaining hive-style + slash + single-field divergence is already noted in the inline discussion. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here.
cc @yihua
voonhous
left a comment
voonhous
left a comment
There was a problem hiding this comment.
Reviewed the formatter change against its callers (SimpleKeyGenerator/ComplexKeyGenerator/CustomKeyGenerator, SqlKeyGenerator, HoodieDatasetBulkInsertHelper, SparkHoodieTableFileIndex) and the Avro path in KeyGenUtils. The core fix is right: Avro = Row = InternalRow for every non-hive combination probed (single field, multi field, null/empty, a-b/c with encoding, CustomKeyGenerator per-field), no revert of #17787/#18195, and UTF8String.replace returns this on no match. Inline comments cover the rest.
One note for a follow-up issue rather than this PR: SlashEncodedDayPartitionValueExtractor (auto-inferred for slash tables by HoodieTableConfigUtils:110-112) throws on any path that is not 3 segments, so hive sync of a slash table containing the __HIVE_DEFAULT_PARTITION__ directory this test now creates will fail.
- assert the InternalRow path for a null partition value at keygen level. [[KeyGeneratorTestUtilities#getInternalRow]] builds a flat GenericInternalRow, so the nested "nested_col.prop1" value stays a Row and reading it back as a struct fails; the conversion goes through Spark's CatalystTypeConverters instead. A null on a top-level field is not an option either -- every other field of the example schema is non-nullable and HoodieUnsafeRowUtils rejects it before the formatter is reached. - assert Row + InternalRow for CustomKeyGenerator, which builds one single-field sub-key-generator per partition field. The test partitioned on "timestamp", a long, which cannot survive the conversion into a Row, so it now uses the string-typed "ts_ms". - pin the url-encoding behaviour: encoding runs before the substitution, so an already slash-separated value is escaped rather than turned into directories. - cover a DATE typed partition column across insert and bulk_insert. - project datestr in the row-writer assertion so the read-back and the null row are covered, and drop the deprecated bulk-insert configs for hoodie.spark.sql.insert.into.operation. - drop two formatter assertions already pinned by TestComplexKeyGenerator. - correct the NOTE on the single-field fast path: CustomKeyGenerator is not an exception to it, and getPartitionPath governs the single-field Avro case. - correct the hive-style comment: the mutual exclusion is documented on SLASH_SEPARATED_DATE_PARTITIONING but only enforced by HoodieCatalogTable for SQL options, so df.write and HoodieStreamer still accept the combination.
ce3ab56 to
8d52393
Compare
github-actions
Bot
added
size:L
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19648 +/- ##
=========================================
Coverage 77.83% 77.83%
- Complexity 33140 33141 +1
=========================================
Files 2527 2527
Lines 139942 139940 -2
Branches 16849 16848 -1
=========================================
Hits 108926 108926
- Misses 23396 23397 +1
+ Partials 7620 7617 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR fixes slash-separated date partitioning on the Spark row-writer path by routing the single-field value through handleEmpty/tryEncode and delegating dash→slash substitution to a per-formatter replaceDashesWithSlashes, and drops the substitution from the multi-field branch to match the Avro KeyGenUtils path. I traced the ordering parity against KeyGenUtils#getPartitionPath/getRecordPartitionPath, the CustomKeyGenerator per-field sub-keygen behavior, and the two formatter subclasses, and the change looks correct and consistent across the Avro, Row, and InternalRow paths. No new issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
|
Azure build 16390 is red on one unrelated test: Everything else in that phase passed (527 succeeded, 1 failed), including all four |
5 participants
Describe the issue this Pull Request addresses
Slash-separated date partitioning did not work on the Spark row-writer path. The single-field
fast path in
PartitionPathFormatterBase#combinecast the value toStringbefore substitutingdashes, so the
UTF8Stringformatter backing theInternalRowwrite path threw aClassCastException, and a null partition value threw aNullPointerExceptionon both stringflavors because the branch bypassed
handleEmpty.Summary and Changelog
PartitionPathFormatterBase#combinenow runs the single-field value throughhandleEmptyandtryEncodelike every other branch, and delegates the substitution to a newreplaceDashesWithSlashesimplemented by each formatter on its own string representation --String#replace(char, char)andUTF8String#replace-- instead of casting.KeyGenUtils#getRecordPartitionPath, which guards it on a single partition field.TestPartitionPathFormatterasserting both formatters agree acrossslash/encode/hive-style/null combinations; keygen-level Row and InternalRow asserts in
TestSimpleKeyGeneratorandTestCustomKeyGenerator; and row-writer plus DATE-typed partitioncolumn cases in
TestSlashSeparatedPartitionValue.No code was copied.
Impact
Records with a null or empty date partition value, and every record written through the row
writer, no longer fail on a slash-partitioned table.
Three consumers of the
Stringformatter now agree with what the Avro writer has alwaysproduced, rather than changing any layout:
SparkHoodieTableFileIndex#composeRelativePartitionPath(query-side prefix pruning) andRowRecordKeyExtractor(bucket routing) previously skippedhandleEmptyandtryEncodeon thesingle-field slash path, while
KeyGenUtilsalways applied both.written by the Avro path, since the row-writer path could not produce one at all.
directory.
Scope is Spark-only -- no Flink or Java-client code reads
hoodie.datasource.write.slash.separated.date.partitioning.No public API, config or storage-format change.
Risk Level
low
The formatter is exercised by the new unit tests across both string flavors, and the SQL-level
suite covers insert, bulk_insert, null values and DATE partition columns. Follow-ups filed during
review for the configurations this PR deliberately does not change: #19666 (multi-field),
#19667 (TIMESTAMP), #19668 (hive sync of the default-partition directory) and #19669
(hive-style plus slash outside SQL DDL).
Documentation Update
none
Contributor's checklist