fix(utilities): include metadata table index-init instants in the record index validation snapshot - #19395
Conversation
…ord index validation snapshot HoodieMetadataTableValidator reads the metadata table with a time-travel snapshot anchored to the data table's latest completed commit. On a table version 6 metadata table the partition-initialization deltacommits are that same data instant with a three-digit suffix appended (010 for FILES, 011 for RECORD_INDEX), and Hudi compares instants as strings, so those derived instants sort after the bare data instant and fall outside the snapshot. When the data table has no commit newer than the initialization instant, every record index file slice is filtered out, the index reads back empty, and the validator reports 100% of the data table's keys as missing from it. This is permanent for a table that has stopped receiving writes; cleans and rollbacks do not help because getWriteTimeline() only whitelists commit, deltacommit, compaction, logcompaction and replacecommit. Table version 8 and above are unaffected: generateUniqueInstantTime derives the init instants from SOLO_COMMIT_TIMESTAMP, which sorts below every data instant. Advance the instant used for the metadata table read by one millisecond. That is strictly greater than any <instant><suffix>, which shares the whole 17-character prefix, while remaining a valid yyyyMMddHHmmssSSS instant - the time travel option runs the value through formatQueryInstant, which rejects anything else. The data table side is unchanged, so both sides stay pinned to the same data instant. Fixes both validateRecordIndexContent and validateRecordIndexCount; the latter carried the same defect, masked only because the content check shadows it.
3328c52 to
b4e7b44
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR fixes HoodieMetadataTableValidator falsely reporting all record keys as missing on table-version-6 tables by advancing the metadata-table time-travel bound one millisecond past the data instant, so the suffixed partition-init and maintenance deltacommits fall inside the queried snapshot. The boundary reasoning holds under string comparison, arbitrary suffix lengths, and rollovers, and the tests cover both the count and content validation paths plus the non-timestamp fallback. No issues flagged from this automated pass; a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19395 +/- ##
============================================
+ Coverage 72.49% 72.52% +0.02%
- Complexity 32888 32896 +8
============================================
Files 2574 2574
Lines 149010 149015 +5
Branches 18749 18749
============================================
+ Hits 108032 108075 +43
+ Misses 32513 32479 -34
+ Partials 8465 8461 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The one red check here, Failing test: Why it shouldn't be reachable from this PR:
What I checked locally: Stronger signal: on a separate CI system running the same 1.x code, this exact test failed and then passed on a re-run of the identical commit with no code change in between. I can't link that run since it isn't public, so please weigh it accordingly — the verifiable parts above are the module-dependency argument and the green master run at the same base. Everything else on this PR is green, including all Flink, Spark, bundle, docker and integration-test shards. |
4 participants
Describe the issue this Pull Request addresses
HoodieMetadataTableValidatorreports 100% of a table's record keys as missing from the recordindex on table version 6 tables whose record index is complete and correct on storage:
The validator reads the metadata table with a time-travel snapshot anchored to the data table's
latest completed commit:
On a table version 6 metadata table, the partition-initialization deltacommits are that same data
instant with a three-digit suffix appended —
HoodieBackedTableMetadataWriterTableVersionSix#createIndexInitTimestamp:so
FILESbecomes<instant>010andRECORD_INDEXbecomes<instant>011. Hudi compares instants asstrings (
InstantComparison#LESSER_THAN_OR_EQUALSisc1.compareTo(c2) <= 0), so those derivedinstants sort after the bare data instant:
When the data table has no commit newer than the initialization instant, the snapshot therefore
excludes every record-index file slice —
HoodieFileGroup#getLatestFileSliceBeforeOrOnfilters themall out and the record index reads back empty.
It reproduces whenever a version 6 metadata table is initialized on a table that already has commits
and no further commit follows: the window between a metadata table (re)bootstrap and the next write,
and permanently for a table that has stopped receiving writes. Cleans and rollbacks do not clear it,
because
getCommitsAndCompactionTimeline()resolves togetWriteTimeline(), which whitelists onlycommit,deltacommit,compaction,logcompactionandreplacecommit.Table version 8 and above are not affected —
HoodieBackedTableMetadataWriter#generateUniqueInstantTimederives the init instants from
SOLO_COMMIT_TIMESTAMPviainstantTimePlusMillis, which sorts belowevery real data instant. This fixes the reader for the version 6 tables that predate that change.
Summary and Changelog
Users running
HoodieMetadataTableValidatoragainst table version 6 tables no longer get spuriousrecord-index validation failures. Genuine record-index divergence is still reported.
Advance the instant used for the metadata-table read by one millisecond, via a new
metadataTableInstantFor(String)helper. That is strictly greater than any<instant><suffix>—those share the whole 17-character prefix and so compare lower — while remaining a valid
yyyyMMddHHmmssSSSinstant. The valid-instant part matters: the time travel option runs the valuethrough
HoodieSqlCommonUtils#formatQueryInstant, which accepts onlyyyyyMMddHHmmssSSS,yyyy-MM-dd HH:mm:ss.SSSoryyyy-MM-ddand throwsIllegalArgumentExceptionon anything else, so asynthetic 20-character bound is not an option. Non-timestamp instants (legacy or test instants such as
100) are passed through unchanged.The data-table side of the comparison is untouched, so both sides stay pinned to the same data instant.
Both affected reads are fixed:
validateRecordIndexContent→getRecordLocationsFromRLIvalidateRecordIndexCount— the same defect, previously masked because the content check shadows itvia the
else ifinvalidateRecordIndexNot changed, and why:
filespartition —validateFilesInPartitionreads throughHoodieBackedTableMetadata, not theSpark datasource, so it never applied the time-travel bound
latestCompletedCommittoo, but load the data table, so theyare correct as-is
No code was copied.
Impact
No public API, config, or output-format change. No behaviour change for tables that receive commits
after metadata table initialization, and none for table version 8+.
Risk Level
low
Verified red→green with a new test that reproduces the state: a table written with
hoodie.write.table.version=6and the metadata table disabled, then bootstrapped out of band throughSparkMetadataWriterFactory— so the version 6 writer is selected, as in production — leaving the initinstants at
<latestDataCommit>010/011while the data table's latest completed commit stays<latestDataCommit>. The test assertsHoodieTableVersion.SIXexplicitly so it cannot silently passif a future change stops producing a version 6 table. Parameterized over both record-index validation
paths.
Before the fix:
Validation of record index content failed: 50 keys (total 50) ... Record Index: <empty>Validation of record index count failed: 0 entries from record index metadata, 50 keys from the data tableAfter the fix both pass, and the full
TestHoodieMetadataTableValidatorclass is green (44/44) onTemurin 11.
The test asserts on
validateRecordIndexdirectly rather than only onrun(). That is deliberate:doMetadataTableValidationcatches any non-HoodieValidationExceptionand returnstrue, reporting afailed read as a successful validation, so an assertion on
run()alone cannot tell a genuine passfrom a swallowed failure. That swallow is pre-existing and is not addressed here; it looks worth a
separate fix, since it can mask real validation errors in production the same way.
Documentation Update
none
Contributor's checklist