test(spark): add incremental read-path relation coverage - #19404
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19404 +/- ##
============================================
- Coverage 75.56% 74.75% -0.81%
+ Complexity 32651 32383 -268
============================================
Files 2574 2576 +2
Lines 142995 143558 +563
Branches 17530 17776 +246
============================================
- Hits 108051 107320 -731
- Misses 26908 28174 +1266
- Partials 8036 8064 +28
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 is a test-only PR that adds coverage for the Spark incremental read-path relations — a new incr.path.glob per-partition filtering test parametrized over table type and version, plus version-8 cases for the legacy streaming source. I traced the glob-to-partition mappings (/2016/*/*/* → 2016/03/15, etc.), the commit boundaries, and the empty-result branch, and the assertions line up correctly. 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. One redundant assertion to remove; everything else is clean.
cc @yihua
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds test-only coverage for the Spark incremental read-path relations. It adds TestIncrementalReadWithPathGlob to exercise the incr.path.glob filtering branch across COW/MOR and table versions 6 and 8, and parametrizes TestStreamingSource by table version to reach the V2 relations. No issues flagged from this automated pass; a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
The assertEquals map comparison already implies the per-partition sums are equal, so the follow-up assertTrue can never catch a distinct failure. Remove it and the now-unused assertTrue import.
1c1a978 to
60db2bf
Compare
voonhous
left a comment
voonhous
left a comment
There was a problem hiding this comment.
Did a deep pass over these relations' git history and ran the new tests locally: all 4 glob cases and both v8 streaming cases pass, and the routing claims check out (COW batch incremental really does go through the MOR relations' filterFileSlices; v8 + FGR-off really drives the V2 relations). Inline comments below, ordered major -> nit: two asks on the new tests themselves, plus historically-motivated coverage gaps in the same relations that are either cheap to close in this diff or worth a tracked follow-up.
| * below EIGHT) and [[HoodieStreamSourceV2]] (table version EIGHT and above), taken when the file | ||
| * group reader is disabled. This drives the standalone incremental relations | ||
| * ([[IncrementalRelationV1]] / [[MergeOnReadIncrementalRelationV1]] for version 6, | ||
| * [[IncrementalRelationV2]] / [[MergeOnReadIncrementalRelationV2]] for version 8) via `getBatch`, |
There was a problem hiding this comment.
Follow-up suggestion: the fallback/full-table-scan blocks of these two standalone relations (IncrementalRelationV1.scala:235-272, IncrementalRelationV2.scala:225-259, including the parallelized file-existence probe from #10480) are reachable by no test in the repo, and they are most of the missing 47%/51% coverage this PR's description cites. The cheapest close is not via streaming: TestLegacyParquetReadPath.testCowIncrementalReadEqualsFileGroupReader already constructs both relations directly (lines 317-319); a variant that sets INCREMENTAL_FALLBACK_TO_FULL_TABLE_SCAN=true after aggressive cleaning and asserts both still equal the file-group-reader result would cover it in ~15 lines. Fine to defer -- if so, please file the follow-up issue.
There was a problem hiding this comment.
Deferred to #19443 (item 1) -- extending TestLegacyParquetReadPath with the fallback-after-clean variant is out of scope for this PR.
| import scala.collection.JavaConverters._ | ||
|
|
||
| /** | ||
| * Coverage for the {@code hoodie.datasource.read.incr.path.glob} file-slice filtering branch of the |
There was a problem hiding this comment.
nit: this covers the file-slice copy of the glob only. The standalone COW relations carry a second, independently-written implementation of the same option over regularFileIdToFullPath/metaBootstrapFileIdToFullPath (IncrementalRelationV1.scala:196-207, V2.scala:181-192, including a meta-bootstrap arm the MOR copy lacks), reachable only from the legacy stream sources -- and no test sets INCR_PATH_GLOB on that path. The streaming suite's table is unpartitioned, so the natural home is the direct-construction test in TestLegacyParquetReadPath (lines 317-319). A follow-up issue would do.
There was a problem hiding this comment.
Deferred to #19443 (item 2); agreed the streaming suite is the wrong home for it since its table is unpartitioned.
| } | ||
|
|
||
| test("test cow stream source with legacy file group reader disabled on table version 8") { | ||
| testLegacyIncrementalStreamSource(COPY_ON_WRITE, HoodieTableVersion.EIGHT) |
There was a problem hiding this comment.
nit, feel free to ignore: adjacent gap while you are raising IncrementalRelationV2 coverage -- INCREMENTAL_READ_SCHEMA_USE_END_INSTANTTIME is set by no test in the repo (grep hits only DataSourceOptions.scala and the two relations), so the useEndInstantSchema=true branch (IncrementalRelationV1.scala:121-129, V2.scala:106-114, from HUDI-1301/#2125) is uncovered. One extra option on the direct-construction test in TestLegacyParquetReadPath would cover ~10 lines per relation. Optional / follow-up.
- MOR glob arms write log-only file slices via the in-memory index and assert merged upsert values plus inputFiles paths per glob - thread enableInlineCluster through addDataToQuery and cluster in the legacy streaming test to reach the replaced-file filtering of the legacy incremental relations - pin the legacy (non file-group-reader) plan shape in the streaming test - add a TestIncrementalReadWithFullTableScan case pinning the divergent v6/v8 default of incr.fallback.fulltablescan.enable on a cleaned span
github-actions
Bot
added
size:L
5 participants
Describe the issue this Pull Request addresses
The Spark incremental read-path relations are under-covered. The four relations in
hudi-spark-datasource/hudi-spark-commonsit well below the rest of the module:IncrementalRelationV2around 51%,IncrementalRelationV1around 47%,MergeOnReadIncrementalRelationV2around 62%, andMergeOnReadIncrementalRelationV1around 74%. Two branches in particular had no coverage: thehoodie.datasource.read.incr.path.globfile filtering used by the batch incremental relations, and the version-8 legacy streaming path that drivesIncrementalRelationV2.Summary and Changelog
This is a test-only change; no production code is modified.
TestIncrementalReadWithPathGlob(functional). It writes one partition per commit, then runs incremental queries withincr.path.globrestricting to each partition and to a partition that does not exist, asserting the exact per-partition row counts. It is parametrized over table type (COW, MOR) and table version (6, 8). Both the COW and MOR batch incremental read routes build aHoodieIncrementalFileIndexbacked byMergeOnReadIncrementalRelationV1(version 6) /MergeOnReadIncrementalRelationV2(version 8), so these cases pinfilterFileSlicesand its empty-result branch for both relations, withincludeLogFilesboth off (COW) and on (MOR).TestStreamingSource. The existing legacy-streaming test (file group reader disabled) was version 6 only, which reachesIncrementalRelationV1/MergeOnReadIncrementalRelationV1but never the V2 relations. Parametrized it by table version and added version-8 cases, so the version-8 legacy streaming path throughHoodieStreamSourceV2drivesIncrementalRelationV2(COW) andMergeOnReadIncrementalRelationV2(MOR).Deliberate overlap and how duplication was avoided:
TestCOWDataSourceStoragealready exercisesincr.path.globfor COW at the default table version and asserts total counts only. The new glob test does not reproduce that. It extends the glob coverage to table version 6 and to MOR, and pins the per-partition result (the partition of every returned row) plus the glob-matches-nothing empty branch, so it fails if the file filtering is wrong rather than merely non-empty. The streaming change folds new version-8 cases into the existing suite and helper rather than adding a near-duplicate class.Impact
None. Tests only.
Risk Level
none
Documentation Update
none
Contributor's checklist