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 1 commit into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19804 +/- ##
============================================
- Coverage 78.29% 78.27% -0.02%
- Complexity 33850 33855 +5
============================================
Files 2541 2541
Lines 141674 141704 +30
Branches 17172 17175 +3
============================================
+ Hits 110917 110923 +6
- Misses 23048 23069 +21
- Partials 7709 7712 +3
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 |
4 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