Skip to content

test(metadata): Add test coverage for deferred RLI init and bulk_insert#18865

Open
nsivabalan wants to merge 1 commit into
apache:masterfrom
nsivabalan:rliDeferInitbulkInsert
Open

test(metadata): Add test coverage for deferred RLI init and bulk_insert#18865
nsivabalan wants to merge 1 commit into
apache:masterfrom
nsivabalan:rliDeferInitbulkInsert

Conversation

@nsivabalan
Copy link
Copy Markdown
Contributor

@nsivabalan nsivabalan commented May 27, 2026

Describe the issue this Pull Request addresses

Follow-up to #18353 (defer RLI init for fresh tables) and #18836 (robust schema resolution during RLI bootstrap). #18836 already lands the schema-fallback fix on dataWriteConfig.getWriteSchema() for the RLI bootstrap path, which also unblocks the deferred-init scenario. This PR adds the test coverage that exercises the deferred RLI init flow end-to-end, including the bulk_insert path, which previously had no dedicated tests.

Tracking issue: #18866 (kept open as the test-coverage gap; the underlying fix is in #18836).

Summary and Changelog

Test-only changes in TestRecordLevelIndex.scala:

Impact

User-facing changes: none. Test coverage only.

Performance impact: none.

Risk Level

low

Documentation Update

none

Contributor's checklist

  • Read through contributor's guide
  • Enough context is provided in the sections above
  • Adequate tests were added if applicable

@github-actions github-actions Bot added the size:M PR with lines of changes in (100, 300] label May 27, 2026
Copy link
Copy Markdown
Contributor

@hudi-agent hudi-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Thanks for the contribution! This PR fixes RLI initialization when the data write config doesn't carry the Avro schema string (deferred init + 2nd commit, or metadata writer constructed for reads), by resolving the schema at the driver with a sensible fallback to tryResolveSchemaForTable. The plumbing is consistent across the RLI init chain and the new tests exercise the bulk_insert defer path end-to-end. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.

cc @yihua

Follow-up to apache#18353 (defer RLI init for fresh tables) and apache#18836 (robust
schema resolution during RLI bootstrap). Adds test coverage for the
deferred RLI init flow:

- Extend testRecordLevelIndex with a deferRLIInit parameter so the
  existing assertions can also exercise the deferred-init configuration.
  When set, the test asserts that after the first save the RLI metadata
  partition is not yet present; the subsequent metadata writer entry
  then triggers the deferred bootstrap and the rest of the test flow
  validates the resulting index.
- Add testPartitionedRecordLevelIndexDefer which drives the deferred
  path via the helper above and verifies compaction afterwards.
- Add testPartitionedRecordLevelIndexDeferWithBulkInsert which issues
  two bulk_insert commits on a fresh table with defer enabled and
  validates the record key -> location mapping after the deferred RLI
  bootstraps on the second commit, including cross-partition negative
  lookups.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@nsivabalan nsivabalan force-pushed the rliDeferInitbulkInsert branch from 8cffc8e to eac93d0 Compare May 27, 2026 18:09
@nsivabalan nsivabalan changed the title fix(metadata): Robust RLI init schema resolution for deferred fresh-table init test(metadata): Add test coverage for deferred RLI init and bulk_insert May 27, 2026
@nsivabalan
Copy link
Copy Markdown
Contributor Author

Rebased on top of latest master.

Note: #18836 (merged before this PR) already lands the schema-resolution fix on the RLI bootstrap path that my original commit was addressing. The Java change in this PR was therefore redundant on rebase and has been dropped — the PR is now scoped to test coverage only:

  • Extend testRecordLevelIndex with a deferRLIInit parameter.
  • Add testPartitionedRecordLevelIndexDefer.
  • Add testPartitionedRecordLevelIndexDeferWithBulkInsert (two bulk_insert commits on a fresh table with defer enabled, validating the record-key -> location mapping after the deferred RLI bootstraps on the 2nd commit).

Copy link
Copy Markdown
Contributor

@hudi-agent hudi-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Thanks for the contribution! This PR is test-only, adding coverage for the deferred RLI init flow (including the previously untested bulk_insert path) as a follow-up to #18353 and #18836. 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. A couple of minor naming nits, otherwise clean and well-commented test code.

cc @yihua

@@ -146,7 +146,8 @@ class TestRecordLevelIndex extends RecordLevelIndexTestBase with SparkDatasetMix
"Metadata files partition count should be lower than data table file count after rebootstrap")
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit: deferRLIInit reads as an imperative verb rather than a boolean state — could you rename it to rliInitDeferred (or isRliInitDeferred) to match the past-participle style used by the sibling parameter streamingWriteEnabled?

- AI-generated; verify before applying. React 👍/👎 to flag quality.

metaClient.getIndexMetadata.get().getIndexDefinitions.get(HoodieTableMetadataUtil.PARTITION_NAME_RECORD_INDEX)),
"RLI should be initialized as partitioned RLI")

// Validate record key -> location mapping for both batches against the data.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit: df is a bit misleading here since .collect() returns an Array[Row], not a DataFrame — something like allRows or tableRows would make the type/intent clearer for validateDFWithLocations callers reading below.

- AI-generated; verify before applying. React 👍/👎 to flag quality.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.77%. Comparing base (1bf6b44) to head (eac93d0).

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18865      +/-   ##
============================================
- Coverage     68.78%   68.77%   -0.02%     
+ Complexity    29142    29118      -24     
============================================
  Files          2514     2514              
  Lines        139914   139914              
  Branches      17184    17187       +3     
============================================
- Hits          96240    96224      -16     
- Misses        35903    35910       +7     
- Partials       7771     7780       +9     
Flag Coverage Δ
common-and-other-modules 44.32% <ø> (-0.01%) ⬇️
hadoop-mr-java-client 44.91% <ø> (-0.01%) ⬇️
spark-client-hadoop-common 48.22% <ø> (-0.01%) ⬇️
spark-java-tests 49.34% <ø> (-0.03%) ⬇️
spark-scala-tests 45.27% <ø> (-0.01%) ⬇️
utilities 37.44% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 13 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hudi-bot
Copy link
Copy Markdown
Collaborator

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M PR with lines of changes in (100, 300]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants