[HUDI-1610] Add SCALAR timestamp type coverage to TestCOWDataSource - #19945
nanjeshramesh wants to merge 1 commit into
Conversation
PR apache#2431 added support for leveraging Spark SQL's partitionBy() clause with TimestampBasedKeyGenerator, and minimal tests were added to TestCOWDataSource for it, but only the EPOCHMILLISECONDS timestamp type is exercised at that write/read integration level. Adds testSparkPartitionByWithTimestampBasedKeyGeneratorScalar, covering the SCALAR timestamp type with an explicit INPUT_TIME_UNIT, a genuinely different code path in TimestampBasedAvroKeyGenerator (unit-based conversion rather than an implied epoch unit) that had no coverage at this integration level. TestTimestampBasedKeyGenerator already unit-tests SCALAR at the key-generator layer directly, but that does not exercise the Spark DataFrame write path, config plumbing, and partition path resolution together the way this test class does for the other timestamp types. The new test mirrors the existing testSparkPartitionByWithTimestampBasedKeyGenerator immediately above it: same data, same assertion shape, same parameterization over AVRO/SPARK record types. Since current_ts is generated in epoch milliseconds, SCALAR with INPUT_TIME_UNIT=MILLISECONDS is expected to partition identically to the existing EPOCHMILLISECONDS test, which the assertion confirms.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR adds a parameterized test in TestCOWDataSource covering the SCALAR timestamp type with an explicit INPUT_TIME_UNIT through the Spark partitionBy() + TimestampBasedKeyGenerator path. I traced the SCALAR branch in TimestampBasedAvroKeyGenerator and confirmed MILLISECONDS yields an identity conversion on the epoch-millis current_ts field, so the assertion against the yyyyMMdd UDF is consistent. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. Code looks clean overall — one minor naming nit, no blocking issues.
cc @yihua
| assertTrue(recordsReadDF.filter(col("_hoodie_partition_path") =!= udf_date_format(col("current_ts"))).count() == 0) | ||
| } | ||
|
|
||
| @ParameterizedTest |
There was a problem hiding this comment.
🤖 nit: the method name doesn't make it clear this is testing the SCALAR + MILLISECONDS combo specifically vs just "scalar" generally — maybe testSparkPartitionByWithTimestampBasedKeyGeneratorScalarMillis to mirror the EPOCHMILLISECONDS test name pattern.
This branch has not been deployed
3 participants
Describe the issue this Pull Request addresses
Closes #14753.
PR #2431 added support for using Spark SQL's
partitionBy()clause withTimestampBasedKeyGenerator, and minimal tests were added toTestCOWDataSourcefor it, but the issue asks for more coverage with respect to the timestamp based key gen than what exists there today.Summary and Changelog
Adds
testSparkPartitionByWithTimestampBasedKeyGeneratorScalar, covering theSCALARtimestamp type with an explicitINPUT_TIME_UNIT. This exercises a genuinely different code path inTimestampBasedAvroKeyGeneratorthan the existingEPOCHMILLISECONDStest right above it:SCALARtreats the field as a raw numeric value that needs an explicit unit to convert to a timestamp, rather than the unit being implied by the type name.TestTimestampBasedKeyGeneratoralready unit-testsSCALARat the key generator layer directly, but that does not exercise the Spark DataFrame write path, config option plumbing, and partition path resolution together the way this test class does for the other timestamp types, so this closes a real gap specific toTestCOWDataSource.The new test mirrors the existing
testSparkPartitionByWithTimestampBasedKeyGeneratorimmediately above it: same data, same assertion shape, same parameterization over AVRO/SPARK record types. Sincecurrent_tsis generated in epoch milliseconds,SCALARwithINPUT_TIME_UNIT=MILLISECONDSis expected to partition identically to the existingEPOCHMILLISECONDStest, which the assertion confirms.Impact
None. Test-only change, no production code touched.
Risk Level
none
Documentation Update
none
Contributor's checklist