Repository navigation
fix(processor): keep the child's selection vector intact in hash join build - #1119
Merged
Merged
Conversation
… build JoinHashTable::appendVectors discards null keys by compacting the key state's selection vector in place, and HashJoinBuild never restored it. Producers such as factorized table and aggregate scans write their next batch through that selection vector after only resetting its size, so every batch after one with a null key was read through the stale compacted positions: one key was lost and another appended twice, while the never written slot kept the null. Results depended on where the null fell, hence on data layout and thread count. When a key may contain nulls, build now appends through a copy of the key selection vector and restores the child's afterwards, as HashJoinProbe already does. Keys with a no-null guarantee skip the copy. Since LadybugDB#1018 (selective correlated-optional unnest) a chained OPTIONAL MATCH puts the null node of a leg that found nothing on a build side; correlated EXISTS, non-equality correlated OPTIONAL MATCH and updates that swap the join sides were already affected. Fixes LadybugDB#1109 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
@zahariash there is some dangling text at the end of the PR description that I can't understand. Also does this PR fix #1121? |
Contributor
Author
Contributor
|
Thanks. This looks good to go. Potential follow-up:
|
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.
This work was produced with the help of language models.
Fixes #1109.
An
OPTIONAL MATCHthat follows one that found nothing returned wrong rows: some(a, c, m)rows came back twice and other comments lost their match, with no error. The nullcof the empty leg ends on a hash join build side.JoinHashTable::appendVectors(src/processor/operator/hash_join/join_hash_table.cpp) drops null keys throughValueVector::discardNull, which compacts the key state's selection vector in place, andHashJoinBuildnever restored it. The producers below the build,FactorizedTable::readFlatColToUnflatVectorand the aggregate hash table scan, write their next batch through that same selection vector after only resetting its size, so every batch after the one with the null was read through stale, compacted positions: one slot is never written and keeps the null, which is dropped again, and one slot is written twice, so one key is lost and another is appended twice. Where the null falls decides how many batches follow it, hence the dependence on data layout and thread count in the issue's table.Since #1018 (selective correlated-optional unnest) a chained
OPTIONAL MATCHleg is planned as a correlated subplan whose outer bindings, including the null node of a leg that found nothing, land on a build side. CorrelatedEXISTS, non-equality correlatedOPTIONAL MATCHand updates that swap the join sides were affected before #1018 too.Fix.
HashJoinBuildinheritsSelVectorOverWriter. When a key vector carries no no-null guarantee,executeInternalappends through a copy of the key state's selection vector and restores the child's afterwards, the patternHashJoinProbealready uses for its key state. Keys with the guarantee skip the copy;discardNullreturns early on the same guarantee, so there is nothing to restore for them.Cost. User-space instruction counts (
perf stat -e instructions:u, deterministic to <0.01%), 1 thread, this PR againstmain:OPTIONAL MATCH(the issue's shape)EXISTSWithout the no-null shortcut the worst case was +2.0%. CPU cycles are +1-3% on the hash-join queries; a profile attributes about 0.26% to
HashJoinBuild::executeInternaland spreads the rest over unrelated functions in both directions (SUM's int128 add, malloc/free and the progress bar up;restoreSelVector, probe and projection down), which is consistent with code placement rather than added work. Wall-clock A/A noise on this machine is about ±0.5%.Before and after. The issue's dataset (65 persons with 500 comments each, except person 32),
threads=1:mainOPTIONAL MATCH,count(*)count(m) <> 1OPTIONAL MATCH ... WHERE m.id <= a.idEXISTS { MATCH (c)-[:replyOf]->(m) WHERE m.id <= a.id }OPTIONAL MATCH (c)-[:replyOf*1..2]->(m)count(DISTINCT c)The issue's thread x layout grid (person 32, 64 or 0 without comments; 1, 2, 3, 4 and default threads; 5 runs each) reproduced the issue's table on
main, up to 33073 at 3 threads; with this PR every cell is 32001.Tests.
test/test_files/issue/issue1109.testruns the six queries above on the issue's dataset atthreads=1. All six fail without the source change.ctest(make test-build-release): 0 of 2861 failed.Measurement setup, profile and the grid
Instruction counts: a synthetic graph with 2M comments, 100k persons and 1M
knowsedges (Comment,Person,Post,hasCreator,replyOf,knows, loaded withCOPY); each query repeated in one shell session atCALL threads=1,perf stat -e instructions:uover the process. The rows of the table:MATCH (c:Comment)-[:hasCreator]->(p:Person) WITH c, p MATCH (c)-[:replyOf]->(m:Post) RETURN sum(p.id + m.id)(a flat-key build called once per comment),MATCH (a:Person) OPTIONAL MATCH (a)<-[:hasCreator]-(c:Comment) OPTIONAL MATCH (c)-[:replyOf]->(m:Post) RETURN count(*), count(m),MATCH (p:Person) WHERE EXISTS { MATCH (p)<-[:hasCreator]-(c:Comment) WHERE c.id < p.id * 20 } RETURN count(*), and for the last rowMATCH (c:Comment) WHERE c.id % 3 = 0 RETURN sum(c.id), count(*)andMATCH (c:Comment) WITH c.id % 1000 AS k, count(*) AS n RETURN sum(n * k). Three binaries:main, the fix without the no-null shortcut, and this PR. Instruction counts repeat to <0.01% between runs, which is why they are the headline numbers; the +1-3% is CPU cycles on the hash-join queries and the ±0.5% is the wall-clock spread between two runs of the same binary. The profile isperf recordonmainand on this PR, compared per symbol.The grid is the issue's reproducer: for each layout (person 32, 64 or 0 without comments) a fresh database, then the chained
OPTIONAL MATCHfive times at 1, 2, 3, 4 and the default thread count, reporting the distinctcount(*)values per cell.