Repository navigation
fix(processor): do not compact flat join keys' selection vectors on null - #1141
Merged
Merged
Conversation
JoinHashTable dropped null keys with ValueVector::discardNull on every key. For a flat key this set its data chunk's selection size to 0, and each side of the join restored only one key state: the build the unflat key state, the flat probe path keyVectors[0]'s. Flatten writes each next row through that same selection vector without resetting its size, so with two keys in different data chunks every row after a null in the other key was lost, on either side of the join. LEFT joins carried a hack that reset the key sizes to 1; COUNT joins had none. A null flat key now only rules the row out, without touching its state; only the unflat key state is compacted, and the side that owns it restores it. appendVectors no longer ignores that no row qualifies, so rows with a null flat key cannot enter the hash table. On a null key, probe also clears the chain left by the previous probe: with a flat key on the unflat probe path, matchUnFlatKey would otherwise follow it now that the key keeps its selection size. The flat probe path no longer needs to save and restore its key state per row, and the LEFT join hack goes away. This also fixes a correlated COUNT subquery counting 1 for a null outer row and crashes in other COUNT subquery shapes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ctors JoinHashTable::appendVectors discards null keys by compacting the unflat key state's selection vector, which the child writes its next batch through, and HashJoinBuild worked around that by appending through a copy and restoring the child's afterwards. appendVectors now makes the copy and restores the child's selection vector itself, so the change and its undo sit in one function and HashJoinBuild no longer needs SelVectorOverWriter. The copy is still skipped when every unflat key has a no-null guarantee. The probe side keeps its save and restore in the operator: probe() fills probedTuples by position in the compacted selection, and the probe and the operators above it read through that selection until the next batch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Thank you! |
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
2 participants
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.
Follow-up to #1119, answering the review comment on it. It also fixes the
COUNT { }failure mentioned in that thread, which turned out to be the same bug.Bug.
JoinHashTabledropped null keys by compacting every key's selection vector, but each side restored only one key state. With two keys in different data chunks, every row after a null in the other key was lost, on either side of the join. This shows up as wrong results and segfaults: a correlatedCOUNT { }subquery after anOPTIONAL MATCHthat found nothing returns{1|32001}instead of{0|1, 1|32000}.Fix. Flat keys are never compacted: a null flat key just rules the row out. Only an unflat key state is ever compacted; the planner allows at most one per side, and each side already restores it. This also removes the per-row save/restore in the flat probe path (the "all keys' states should be restored" TODO) and the
LEFTjoin hack.Moving the save/restore into
JoinHashTable. The build side now does it inappendVectors(second commit, separate so it can be dropped). The probe side cannot: the operators read through the compacted selection afterprobe()returns.Cost. Instruction counts drop 0.4-2.5% on hash-join queries and are unchanged elsewhere (details below).
Instruction counts, 1 thread, against `main`, same graph and queries as #1119
OPTIONAL MATCH(#1109's shape)EXISTSWITHThe flat probe path no longer saves and restores per row. The second commit adds 0.2-0.35% over the first on flat-key builds (one more non-inlined call per append). Cycle differences are within noise.
Tests. The new
null_keys.testfails onmain(wrong results, segfault) and passes here;ctest: 0 of 2934 failed.