fix: avoid top panic on null varlen copy#24063
Merged
mergify[bot] merged 7 commits intomatrixorigin:mainfrom Apr 7, 2026
Merged
fix: avoid top panic on null varlen copy#24063mergify[bot] merged 7 commits intomatrixorigin:mainfrom
mergify[bot] merged 7 commits intomatrixorigin:mainfrom
Conversation
Guard compare copy and vector copy against null varlen rows so Top heap replacement does not dereference stale varlena metadata from null values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
LeftHandCold
requested review from
XuPeng-SH,
aunjgr and
ouyuanning
as code owners
Review Summary by QodoFix panic on null varlen copy in Top operator
WalkthroughsDescription• Guard null varlen row copies against stale metadata dereferencing • Restructure Copy logic to handle null values before data operations • Clear varlena headers when copying null values to destination • Add comprehensive tests for null varlena copy scenarios Diagramflowchart LR
A["Copy Operation"] --> B{"Is Source Null?"}
B -->|Yes| C["Clear Varlena Header"]
C --> D["Mark Destination Null"]
D --> E["Return Early"]
B -->|No| F["Copy Data Normally"]
F --> G["Mark Destination Non-Null"]
G --> H["Return Success"]
File Changes1. pkg/compare/arraycompare.go
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewⓘ The new review experience is currently in Beta. Learn more |
XuPeng-SH
approved these changes
Apr 5, 2026
Contributor
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Review Summary: LGTM ✅
Reviewed with three parallel agents (correctness, caller impact, varlena safety).
Fix Assessment
Root cause: Vector.Copy() checked nulls AFTER the data copy. For null varlen rows with stale Varlena offset/len metadata, GetByteSlice() would do area[stale_offset:stale_offset+stale_len] → out-of-bounds panic in Top operator.
Fix (3 layers):
- vector.go: Null check moved BEFORE data copy (early return). Zeroes destination varlena header for null rows — good defensive measure.
- strcompare.go / arraycompare.go: Removed redundant
Copycall in grouping-non-null branch. This was both a bug (copied null data with stale metadata) AND a performance waste (non-null rows were copied twice). - Tests: Reproduce the exact bug with deliberately injected stale varlena metadata via
SetOffsetLen(25, 8). End-to-end Top test validates the full path.
Correctness Verification
| Aspect | Status |
|---|---|
| Null check placement (before data copy) | ✅ Correct |
| Removed first Copy in compare types | ✅ Correct — was redundant or buggy |
Varlena zeroing (vva[vi] = types.Varlena{}) |
✅ Good defensive practice |
Unconditional Unset at end of Copy |
✅ Only reachable for non-null source |
| Constant null path handling | ✅ Complete |
| No other compare types need fixing | ✅ Verified — only strCompare/arrayCompare handle varlen |
Caller Impact Analysis
All Vector.Copy() callers benefit from the internal null-safety:
| Caller | Pre-existing null guard? | Safe after fix? |
|---|---|---|
| lockop.go:176 | ✅ Yes | ✅ |
| value_scan.go:45 | ❌ No | ✅ Now safe via internal check |
| on_duplicate_key.go:169,184,192,341 | ❌ No | ✅ Now safe via internal check |
| strcompare/arraycompare | ✅ Fixed in this PR | ✅ |
Minor Observation (not a blocker)
on_duplicate_key.go:341 loops all rows without null pre-check — now safe because Vector.Copy handles nulls internally, but was a latent risk before this fix.
ouyuanning
approved these changes
Apr 5, 2026
aunjgr
approved these changes
Apr 7, 2026
Contributor
Merge Queue Status
This pull request spent 18 minutes 40 seconds in the queue, with no time running CI. Required conditions to merge
ReasonThe pull request #24063 has been manually updated HintIf you want to requeue this pull request, you can post a |
Contributor
Merge Queue Status
This pull request spent 4 minutes in the queue, with no time running CI. ReasonThe pull request #24063 has been manually updated HintIf you want to requeue this pull request, you can post a |
Contributor
Merge Queue Status
This pull request spent 54 minutes 57 seconds in the queue, including 54 minutes 33 seconds running CI. Required conditions to merge
|
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
5 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.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24048
What this PR does / why we need it:
Guard compare copy and vector copy against null varlen rows so Top heap replacement does not dereference stale varlena metadata from null values.