test(spark): add TimestampBasedKeyGenerator tests with various configurations - #19657
test(spark): add TimestampBasedKeyGenerator tests with various configurations#19657zhang-arvin wants to merge 2 commits into
Conversation
…urations Add comprehensive test coverage for TimestampBasedKeyGenerator in TestCOWDataSource, covering scenarios not addressed by the existing minimal test: - EPOCHMILLISECONDS with timezone GMT+8:00 - EPOCHMICROSECONDS - DATE_STRING with timezone configuration - SCALAR with hours time unit Closes apache#14753
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR expands TimestampBasedKeyGenerator coverage in TestCOWDataSource across EPOCHMILLISECONDS (with timezone), EPOCHMICROSECONDS, DATE_STRING (with timezone), and SCALAR-hours configurations. The EPOCHMILLISECONDS, EPOCHMICROSECONDS, and SCALAR cases line up with the keygen's timezone handling, but the DATE_STRING expected-value UDF looks like it may be environment-dependent — see the inline comment. Please take a look at the inline comment, and this should be ready for a Hudi committer or PMC member to take it from here. A few small inconsistencies worth tidying up below.
|
|
||
| // Test 3: DATE_STRING with timezone | ||
| val dateStrOutFmt = "yyyy-MM-dd HH" | ||
| val dateStrInFmt = "yyyy-MM-dd HH:mm:ss" |
There was a problem hiding this comment.
🤖 I think this UDF may be timezone-dependent and won't match the keygen. Since TIMESTAMP_TIMEZONE_FORMAT is GMT+8:00, the keygen builds its input formatter withZone(GMT+8), so it parses the string as GMT+8 and formats in GMT+8 — the wall-clock hour round-trips. Here DateTime.parse(s, DateTimeFormat.forPattern(dateStrInFmt)) has no zone, so it parses in the JVM default zone before .withZone(GMT+8) shifts the hour. On a UTC CI box that's an 8-hour skew (keygen ...01 12 vs UDF ...01 20), and "yyyy-MM-dd HH" includes the hour. Could you set the parse zone to GMT+8 (e.g. DateTimeFormat.forPattern(dateStrInFmt).withZone(GMT+8)) so it matches the keygen regardless of JVM timezone?
| .withColumn("current_date_string", | ||
| date_format((col("current_ts") / 1000).cast("timestamp"), "yyyy-MM-dd HH:mm:ss")) | ||
| .withColumn("current_ts_hours", (col("current_ts") / 3600000).cast("long")) | ||
| .withColumn("current_ts_seconds", (col("current_ts") / 1000).cast("long")) |
There was a problem hiding this comment.
🤖 nit: current_ts_seconds is prepared here but isn't referenced in any of the four test cases — could you either add a test that exercises it or drop the column to avoid leaving future readers wondering if there's a missing scenario?
| .withZone(org.joda.time.DateTimeZone.forID("GMT+8:00")) | ||
| .toString(DateTimeFormat.forPattern(dateStrOutFmt).withZone(org.joda.time.DateTimeZone.forID("GMT+8:00")))) | ||
| runTestCase(TestCase("current_date_string", "DATE_STRING", dateStrOutFmt, | ||
| Map("hoodie.keygen.timebased.input.dateformat" -> dateStrInFmt, |
There was a problem hiding this comment.
🤖 nit: the rest of this test uses .key accessors (e.g. TIMESTAMP_TIMEZONE_FORMAT.key) — could you replace "hoodie.keygen.timebased.input.dateformat" with the corresponding constant so all config keys here are refactor-safe?
| new DateTime(java.util.concurrent.TimeUnit.HOURS.toMillis(hours)) | ||
| .toString(DateTimeFormat.forPattern(scalarHoursOutFmt))) | ||
| runTestCase(TestCase("current_ts_hours", "SCALAR", scalarHoursOutFmt, | ||
| Map("hoodie.keygen.timebased.timestamp.scalar.time.unit" -> "hours"), udfScalarHours)) |
There was a problem hiding this comment.
🤖 nit: same as above — "hoodie.keygen.timebased.timestamp.scalar.time.unit" could be replaced with the named constant's .key accessor to match the pattern used for all the other options.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19657 +/- ##
============================================
- Coverage 73.60% 69.21% -4.40%
+ Complexity 31322 29486 -1836
============================================
Files 2525 2525
Lines 139644 139644
Branches 16809 16809
============================================
- Hits 102791 96649 -6142
- Misses 29701 35803 +6102
- Partials 7152 7192 +40
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 the update! The change since my last review normalizes the timezone string in Tests 1 and 3 from \"GMT+8:00\" to \"GMT+08:00\". However, the core issue I raised on the DATE_STRING case (Test 3) is still open: the expected-value UDF parses the date string without a zone, so it's tied to the JVM default timezone and won't match the keygen (which parses the string as GMT+8) on a non-GMT+8 box such as a UTC CI runner — see the inline comment for the one-line fix. The three nits from the prior pass (the unused current_ts_seconds column and the two hardcoded config-key literals for input dateformat and scalar time unit) also appear untouched, but those are non-blocking. Please take a look at the inline comment, and this should be ready for a Hudi committer or PMC member to take it from here.
| // Test 3: DATE_STRING with timezone | ||
| val dateStrOutFmt = "yyyy-MM-dd HH" | ||
| val dateStrInFmt = "yyyy-MM-dd HH:mm:ss" | ||
| val udfDateStrTz = udf((s: String) => |
There was a problem hiding this comment.
🤖 The "GMT+8:00"→"GMT+08:00" rename doesn't address the earlier concern here: DateTime.parse(s, DateTimeFormat.forPattern(dateStrInFmt)) still parses with no zone (JVM default), then .withZone(GMT+08:00) shifts the hour — whereas the keygen's input formatter is .withZone(inputDateTimeZone=GMT+08:00), so it parses the string as GMT+8 (hour round-trips). On a UTC CI box that's an 8h skew and the yyyy-MM-dd HH assertion fails. Could you set the parse zone too, e.g. DateTimeFormat.forPattern(dateStrInFmt).withZone(DateTimeZone.forID("GMT+08:00"))?
2a078e8 to
45131da
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR expands test coverage for TimestampBasedKeyGenerator in TestCOWDataSource, adding cases for EPOCHMILLISECONDS with timezone, EPOCHMICROSECONDS, DATE_STRING with timezone, and SCALAR with hours. No new issues flagged from this automated pass beyond the timezone-dependency concerns already raised in earlier rounds — a Hudi committer or PMC member can take it from here for a final review. One minor duplication worth collapsing, otherwise the test reads clearly.
cc @yihua
| assertTrue(readDF.filter(col("_hoodie_partition_path") =!= tc.expectedPartitionUdf(col(tc.partitionCol))).count() == 0) | ||
| } | ||
|
|
||
| // Test 1: EPOCHMILLISECONDS with timezone GMT+08:00 |
There was a problem hiding this comment.
🤖 nit: tzMillisOutFmt, microsOutFmt, dateStrOutFmt, and scalarHoursOutFmt all hold the same "yyyy-MM-dd HH" value — could you hoist a single val outputDateFmt = "yyyy-MM-dd HH" above the test cases and share it? Makes it easier to experiment with a different format later.
4 participants
Describe the issue this Pull Request addresses
Closes #14753
Summary and Changelog
This PR adds comprehensive test coverage for
TimestampBasedKeyGeneratorinTestCOWDataSource, covering scenarios not addressed by the existing minimal test. The existing test only coversEPOCHMILLISECONDSwithyyyyMMddoutput format.The following additional timestamp configurations are now tested:
Each test case writes data using the datasource API with
partitionBy()and verifies that the generated partition paths match the expected timestamp-based format.Impact
none
Risk Level
low
This change only expands test coverage and does not modify production code.
The risk is limited to test maintenance and expected-value correctness.
Documentation Update
none
Contributor's checklist