refactor(reader): Use reader context API for legacy scanner delete re… - #19804
refactor(reader): Use reader context API for legacy scanner delete re…#19804fhan688 wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19804 +/- ##
============================================
+ Coverage 78.29% 78.31% +0.02%
- Complexity 33850 33910 +60
============================================
Files 2541 2542 +1
Lines 141674 141822 +148
Branches 17172 17258 +86
============================================
+ Hits 110917 111068 +151
+ Misses 23048 23046 -2
+ Partials 7709 7708 -1
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 PR routes legacy log-scanner delete-block reading through HoodieReaderContext so native delete log blocks materialize correctly, lazily building an Avro reader context on the first delete block and threading the physical partition path through the delete callbacks. The tracing checks out (delete ordering semantics, the unchanged data-record metric, and the null-safe metaClient path all look correct); one semantic change around how the delete partition path is derived is worth a second look in the inline comment. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. One naming nit on the implicit assumption baked into resolveDeletePartitionPath.
| if (logFilePaths.isEmpty()) { | ||
| return Option.empty(); | ||
| } | ||
| return Option.of(getRelativePartitionPath( |
There was a problem hiding this comment.
🤖 This changes where the delete's partition path comes from: previously each DeleteRecord.getPartitionPath() was used per-record, now it's derived once from partitionNameOverride or the first log file's parent dir and applied to every delete in the block. For these single-file-group scanners that should always match, but I want to double-check — is there any path (e.g. a legacy delete block whose DeleteRecords carry a partition different from the physical log location) where the stored partition could now differ? @yihua does relying on the log file's physical partition for deletes hold across all read paths here?
| return Option.empty(); | ||
| } | ||
| return Option.of(getRelativePartitionPath( | ||
| new StoragePath(basePath), new StoragePath(logFilePaths.get(0)).getParent())); |
There was a problem hiding this comment.
🤖 nit: could you add a brief inline comment here explaining why get(0) is safe — something like // all log files in a file group share the same parent partition directory? Without it a future reader might wonder whether this silently breaks for multi-directory log paths.
|
please help review, thanks! @cshuo |
| if (deleteReaderContext == null) { | ||
| HoodieTableConfig tableConfig = hoodieTableMetaClient.getTableConfig(); | ||
| TypedProperties mergeProps = ConfigUtils.getMergeProps(payloadProps, tableConfig); | ||
| HoodieReaderContext<IndexedRecord> readerContext = new HoodieAvroReaderContext( |
There was a problem hiding this comment.
Inject the reader context through the scanner builder
The scanner should not construct a HoodieAvroReaderContext internally or derive its reader configuration from the scanner's payload-only properties. The external caller owns the engine and complete runtime configuration, so it is the appropriate place to create the reader context.
Please follow the HoodieFileGroupReader construction pattern: add Builder#withReaderContext(...), have callers such as Hadoop MR realtime readers, the Spark procedure, and tests construct the appropriate context, and pass it into the scanner.
…rs for delete records
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the updates! This revision moves the delete-block reader context out of the scanner and injects it through the builder (withReaderContext) instead of constructing a HoodieAvroReaderContext internally, which directly addresses @cshuo's feedback. The context is now caller-owned, required via checkArgument, and all call sites (Hadoop MR realtime readers, the Spark show-log procedure, the integ-test reader, CLI, and the log-format tests) were updated. I verified there are no stray un-migrated callers and that the injected reader context's instantRange is not consumed during delete-block reading, so there's no consistency trap between the scanner's own instantRange and the injected one. My earlier question about deriving the delete partition path from the physical log location (vs. each DeleteRecord's stored partition) and the small get(0) doc nit remain open but are already posted for a committer to weigh in. No new issues flagged from this automated pass, and a Hudi committer or PMC member can take it from here for a final review.
| readerContext.setTablePath(hoodieTableMetaClient.getBasePath().toString()); | ||
| readerContext.setLatestCommitTime(latestInstantTime); | ||
| readerContext.getRecordContext().setPartitionPath(deletePartitionPathOpt.orElse(null)); | ||
| readerContext.initRecordMerger(mergeProps); |
There was a problem hiding this comment.
This reader context is only used to deserialize native delete records; the scanner uses its own recordMerger for merging. Initializing the reader-context merger can fail for CUSTOM merge mode when hoodie.write.record.merge.custom.implementation.classes is unavailable, even though it will never be used here.
Please remove initRecordMerger and setSchemaHandler. Instead, update HoodieAvroReaderContext#getFileRecordIterator so the isLogFile path uses requiredSchema directly without accessing the schema handler.
github-actions
Bot
added
size:L
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR refactors the legacy log record scanners to materialize delete records through HoodieReaderContext (enabling native delete-log-block reads), derives the delete partition path from the log file location, and threads it through the merged/unmerged delete callbacks. The delete-ordering semantics and partition-path derivation trace correctly against the added tests, and the earlier rounds already flagged the key design questions (reader-context injection and per-record vs. derived partition path). No new correctness issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
5 participants
Describe the issue this Pull Request addresses
Closes #19196.
The legacy log record scanner reads delete blocks through the no-argument
HoodieDeleteBlock#getRecordsToDelete()API. This bypassesHoodieReaderContextand prevents the scanner from correctly reading native delete log blocks, whose records must be materialized through the reader-context API.Summary and Changelog
This PR refactors legacy log scanners to read delete records through
HoodieReaderContext.HoodieReaderContextwhen the first delete block is processed.HoodieDeleteBlock#getRecordsToDelete(HoodieReaderContext).BufferedRecorddelete records.getRecordsToDelete()APIs while retaining them for compatibility and block serialization.Impact
There are no storage-format, configuration, or user-facing behavior changes.
The existing no-argument delete-record APIs remain available but are now deprecated. Reader-context initialization is lazy and only occurs when a delete block is processed.
The protected delete-processing hook used by the legacy scanner implementations now operates on
BufferedRecordand receives the physical partition path explicitly.Risk Level
Low.
The change is scoped to delete-block processing in legacy merged and unmerged log scanners. Existing compatibility APIs and serialization paths are preserved.
The merged, unmerged, and mixed-format delete paths are covered by regression tests. Module compilation and Checkstyle validation also pass.
Documentation Update
None. This is an internal refactoring without new configuration or user-facing behavior.
Contributor's checklist