[core] Match the lookup high level record by position, not by identity - #9279
Open
PDGGK wants to merge 1 commit into
Open
[core] Match the lookup high level record by position, not by identity#9279PDGGK wants to merge 1 commit into
PDGGK wants to merge 1 commit into
Conversation
LookupMergeFunction.getResult scans the candidate buffer twice and re-identifies the high level record with kv == highLevel. Once the candidates exceed lookup.merge-records-threshold they spill to a BinaryBuffer, whose iterator deserializes a new KeyValue on every call, so the second scan never sees the object the first scan returned and the high level record is dropped from the merge. Record the position pickHighLevel chose and compare positions instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
LookupMergeFunction.getResultscans the candidate buffer twice and re-identifies the high level record by object identity:The candidates are held in a
KeyValueBuffer.HybridBuffer, which keeps them in anArrayListuntil there are more thanlookup.merge-records-thresholdof them for the key and then spills to aBinaryBuffer(KeyValueBuffer:91). A spilled buffer hands out a new object on every call:So after the spill the second scan never sees the object the first scan returned.
kv == highLevelis false for every record, and the high level record is dropped from the merge.The threshold counts candidates for a single key in one merge, so this needs a key with more than
lookup.merge-records-thresholdrecords across the files being merged — 1024 by default, lower for anyone who has tuned the option down to bound memory.What it does
LookupMergeFunctionTest.testKeepLowestHighLevel— two high level records, expecting the lower level to win — is the shape that breaks. Run unchanged except that the candidates spill, it returns null instead of the level-1 value: neither record is level 0 and neither matches by identity, so nothing at all reaches the wrapped merge function.In the pipeline this runs through
LookupChangelogMergeFunctionWrapper.getResult(:104-146), and the two halves then disagree. The wrapper holds its own non-nullhighLevelfrom its ownpickHighLevel()call at:106and passes it tosetChangelogas the before image at:142, while the after image it is compared against was merged without that record. For a merge engine that folds the persisted row into the result — partial-update, aggregation — the merged row loses the columns that only the high level record carried, and the changelog describes an update against a base row that was never part of it.What changes
Match the record by its position in the buffer instead of by object identity.
pickHighLevelrecords the index of the record it chose, andgetResultcompares indices on the second scan:Both scans walk the same buffer with no intervening writes, and the buffer iterates in insertion order:
ListBufferover anArrayList, andBinaryBufferover aRowBufferwhosenewIterator()builds a freshRandomAccessInputViewfrom the start of the record segments and reads forward (InMemoryBuffer:119-127). Both spilled flavours are covered by the tests below rather than only by that reading.highLevelIndexis reset inreset()alongside the other per-key state.That same code is why identity can never hold once spilled: the iterator's
getRow()returns a reusedBinaryRow, soBinaryBufferhas tocopy()it before deserialising.This is deliberately not a value comparison: two candidates for one key can share a level, and comparing on
(level, sequenceNumber)would rest on an assumption about sequence numbers that the position does not need.Test evidence
Two tests, each run against both spilled buffer flavours —
ioManager == null, which spills to anInMemoryBuffer, and a realIOManager, which spills to anExternalBuffer:testKeepLowestHighLevelWhenCandidatesHaveSpilled— the existingtestKeepLowestHighLevelscenario withlookup.merge-records-threshold = 1.testPicksTheLowestHighLevelFromTheMiddleWhenCandidatesHaveSpilled— levels 3, 1, 2 inserted in that order, so the answer is neither the first nor the last candidate and an off-by-one in the index bookkeeping would not pass.Mutation control, on a forced clean rebuild of
paimon-core(rm -rf target/classes target/test-classes) so this is not an incremental-build artefact: with the tests kept andLookupMergeFunctionreverted, both new tests fail —Expecting actual not to be null— while the two pre-existing tests in the class stay green, which is the point: they do not spill.Wider run:
*Lookup*Test,*MergeTree*Test,*MergeFunction*Test,KeyValueBufferTestand*Compact*Testacrosspaimon-core— 286 tests, 0 failures.API and Format
No change to any option, on-disk format or public signature. Below the spill threshold the two scans return the same objects and the behaviour is byte-for-byte what it was.