refactor(reader): seal the shared merge path in KeyBasedFileGroupRecordBuffer - #19415
Conversation
…rdBuffer Mark processNextDataRecord and isPartialMergingEnabled as final so that subclasses cannot replace the merge-then-store behavior shared by every buffer in this hierarchy. Subclasses still choose the identifier a record is buffered under, and still specialize block processing and base-record advancement. Adds a reflection-based regression test so the modifiers cannot be dropped silently. Closes apache#16920
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR marks processNextDataRecord and isPartialMergingEnabled in KeyBasedFileGroupRecordBuffer as final to seal the shared merge path against subclass overrides, and adds a reflection-based test to enforce it. 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 #19415 +/- ##
============================================
- Coverage 75.36% 75.10% -0.26%
+ Complexity 32552 32439 -113
============================================
Files 2574 2574
Lines 142994 142995 +1
Branches 17529 17535 +6
============================================
- Hits 107762 107400 -362
- Misses 27182 27605 +423
+ Partials 8050 7990 -60
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
cshuo
left a comment
cshuo
left a comment
There was a problem hiding this comment.
thks for the contribution, left some comments.
…arantees Review feedback: the test name, javadoc and failure message claimed every buffer funnels records through a single merge-then-put path. That is not true and the test did not check it. PositionBasedFileGroupRecordBuffer writes to the protected records map directly when it re-keys entries in fallbackToKeyBasedBuffer and when it overwrites delete markers under COMMIT_TIME_ORDERING, and subclasses can still override block processing. Narrow the claim instead of inventing the invariant: those two paths must not merge, so routing them through processNextDataRecord would change behaviour. Renamed the test to sealedMethodsCannotBeOverridden, reworded its javadoc and message to the exact guarantee (the two methods keep their final modifier), and corrected the same overstatement in the production javadoc, including that a subclass may still set enablePartialMerging while processing a data block. No behaviour change.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR seals processNextDataRecord and isPartialMergingEnabled in KeyBasedFileGroupRecordBuffer with final and adds a reflection test to guard the modifier. I verified the hierarchy — only PositionBasedFileGroupRecordBuffer and SortedKeyBasedFileGroupRecordBuffer extend the class, neither overrides either method, and the external isPartialMergingEnabled caller in CDCFileGroupIterator is unaffected, so this is behavior-preserving. No new issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
7 participants
Describe the issue this Pull Request addresses
Closes #16920 (HUDI-9200).
KeyBasedFileGroupRecordBufferis subclassed byPositionBasedFileGroupRecordBufferandSortedKeyBasedFileGroupRecordBuffer. Both legitimately specialize how log blocks are consumed andhow the base-file iterator is advanced, but the step that actually mutates the buffer — merge the
incoming record against what is already buffered, then store the result — must behave identically for
every buffer in the hierarchy. Today nothing expresses that, so a subclass can silently replace it.
Summary and Changelog
Marks the two methods that no subclass overrides as
final, so the shared merge path cannot bereplaced by a subclass, and documents why:
KeyBasedFileGroupRecordBuffer#processNextDataRecord→final. Subclasses still choose theidentifier a record is buffered under (a record key here, a record position in
PositionBasedFileGroupRecordBuffer), which is the parameter, not the behavior.KeyBasedFileGroupRecordBuffer#isPartialMergingEnabled→final. TheenablePartialMergingflag isowned and toggled by the base buffer while processing data blocks.
TestKeyBasedFileGroupRecordBuffer#keyMergeBehaviorIsSealedAgainstSubclasses, which assertsboth modifiers via reflection.
finalis compiler-enforced, but nothing otherwise stops the keywordfrom being dropped later; the test was confirmed to fail for each method independently before the
change.
The remaining methods (
getBufferType,processDataBlock,processDeleteBlock,containsLogRecord,hasNextBaseRecord,doHasNext) are deliberately left open — each is overridden byPositionBasedFileGroupRecordBufferand/orSortedKeyBasedFileGroupRecordBuffertoday.Note on scope: the ticket is phrased as preventing
PositionBasedFileGroupRecordBufferfrom overridingkey functions, and
PositionBaseddoes not override either method sealed here. If the intent wasinstead to refactor
PositionBasedso it stops overriding the block-processing methods (each of whichcurrently opens with a
if (!getShouldMergeUseRecordPosition()) { super.…; return; }fallback) and thenseal those, that is a larger design change and I'm happy to defer to the ticket's assignee on it. This
PR is the behavior-preserving subset.
Impact
None for users.
finalis enforced at compile time and these are internal reader classes — nothing inorg.apache.hudi.common.table.read.bufferis annotated@PublicAPIClass. No public API, config, oron-disk format change. Making a public method
finalis binary-compatible; it would only affect anout-of-tree subclass that overrides these methods, and no in-tree subclass, test, or mock does.
No performance claim is made: these call sites are already monomorphic and devirtualized by the JIT.
Risk Level
none
Documentation Update
none — no new config, no default value change, no user-facing behavior change.
Contributor's checklist