fix: avoid top panic on null varlen copy by LeftHandCold · Pull Request #24049 · matrixorigin/matrixone

LeftHandCold · 2026-04-03T07:17:30Z

What type of PR is this?

Which issue(s) this PR fixes:

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.

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>

qodo-code-review · 2026-04-03T07:17:49Z

Review Summary by Qodo

Fix panic on null varlen copy in Top operator

🐞 Bug fix

Walkthroughs

Description

• Guard null varlen row copies against stale metadata dereferencing
• Reorganize Copy logic to clear varlena headers before marking null
• Prevent Top heap replacement panic on null varlen values
• Add comprehensive tests for null varlena copy scenarios

Diagram

flowchart LR
  A["Null Varlen Copy"] --> B["Clear Varlena Header"]
  B --> C["Mark Row Null"]
  C --> D["Prevent Stale Metadata"]
  D --> E["Top Operator Safe"]

File Changes

1. pkg/compare/arraycompare.go 🐞 Bug fix +2/-4

Reorganize null check before vector copy

• Reorganized Copy method to only call vector copy when source is not null
• Moved null check before vector copy to prevent dereferencing stale varlena metadata
• Simplified control flow by removing redundant else branches

pkg/compare/arraycompare.go

2. pkg/compare/strcompare.go 🐞 Bug fix +2/-4

Reorganize null check before vector copy

• Reorganized Copy method to only call vector copy when source is not null
• Moved null check before vector copy to prevent dereferencing stale varlena metadata
• Simplified control flow by removing redundant else branches

pkg/compare/strcompare.go

3. pkg/container/vector/vector.go 🐞 Bug fix +16/-5

Guard varlena copy against null metadata

• Added explicit null handling for constant null vectors with varlena types
• Clear varlena header metadata before marking destination as null
• Added early return for null source rows to prevent stale metadata access
• Simplified final null flag logic by removing conditional unset

pkg/container/vector/vector.go

View more (2)

4. pkg/container/vector/vector_test.go 🧪 Tests +32/-0

Add null varlena copy tests

• Added test case for string null with stale varlena metadata
• Added test case for array null with stale varlena metadata
• Both tests verify that Copy correctly handles null rows with invalid offset/len values

pkg/container/vector/vector_test.go

5. pkg/sql/colexec/top/top_test.go 🧪 Tests +39/-0

Add Top operator null varlena test

• Added import for vector package
• Added new test TestTopCopiesNullVarlenaRow to verify Top operator handles null varlena rows
• Test creates batch with null varchar row containing stale metadata and verifies correct handling

pkg/sql/colexec/top/top_test.go

qodo-code-review · 2026-04-03T07:17:51Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

1. issue 24048 SQL test missing ☑ 📎 ☼

Description

Compliance requires an automated regression test that executes the reported SQL query pattern
(including FORCE INDEX, keyset pagination predicate, ORDER BY, LIMIT, FOR UPDATE) to prevent
reintroduction of the panic. The added tests only exercise operator/vector behavior and do not run
the end-to-end SQL pattern, so the specific crash path could return without being caught.

Code

pkg/sql/colexec/top/top_test.go[R100-136]

+func TestTopCopiesNullVarlenaRow(t *testing.T) {
+	tc := newTestCase(
+		t,
+		mpool.MustNewZero(),
+		[]types.Type{types.T_int64.ToType(), types.T_varchar.ToType()},
+		1,
+		[]*plan.OrderBySpec{{Expr: newExpression(0), Flag: 0}},
+	)
+	err := tc.arg.Prepare(tc.proc)
+	require.NoError(t, err)
+
+	bat := batch.NewWithSize(2)
+	bat.Vecs[0] = vector.NewVec(types.T_int64.ToType())
+	err = vector.AppendFixedList(bat.Vecs[0], []int64{2, 1}, nil, tc.proc.Mp())
+	require.NoError(t, err)
+
+	bat.Vecs[1] = vector.NewVec(types.T_varchar.ToType())
+	err = vector.AppendBytes(bat.Vecs[1], []byte("seed"), false, tc.proc.Mp())
+	require.NoError(t, err)
+	err = vector.AppendBytes(bat.Vecs[1], nil, true, tc.proc.Mp())
+	require.NoError(t, err)
+	ws := vector.MustFixedColNoTypeCheck[types.Varlena](bat.Vecs[1])
+	ws[1].SetOffsetLen(25, 8)
+	bat.SetRowCount(2)
+
+	resetChildren(tc.arg, []*batch.Batch{bat, batch.EmptyBatch})
+	result, err := vm.Exec(tc.arg, tc.proc)
+	require.NoError(t, err)
+	require.NotNil(t, result.Batch)
+	require.Equal(t, int64(1), vector.MustFixedColWithTypeCheck[int64](result.Batch.Vecs[0])[0])
+	require.True(t, result.Batch.Vecs[1].GetNulls().Contains(0))
+
+	tc.arg.Free(tc.proc, false, nil)
+	tc.arg.GetChildren(0).Free(tc.proc, false, nil)
+	tc.proc.Free()
+	require.Equal(t, int64(0), tc.proc.Mp().CurrNB())
+}

Evidence
PR Compliance ID 2 requires regression coverage that executes the reported SQL pattern; the added
TestTopCopiesNullVarlenaRow is a unit-style test building batches and invoking vm.Exec directly,
and it does not execute the SQL pattern described in the rule.
Add regression coverage for the reported query pattern to prevent reintroduction of the panic
pkg/sql/colexec/top/top_test.go[100-136]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Compliance requires automated regression coverage that executes the reported SQL query pattern (FORCE INDEX + keyset pagination predicate on `created_at/instance_id/bill_id` + `ORDER BY` + `LIMIT` + `FOR UPDATE`) and asserts it completes without panic.
## Issue Context
The PR adds unit-level tests around `Vector.Copy` and `Top` operator null varlena copying, but does not add a SQL-level test that exercises the exact query pattern described in the compliance checklist.
## Fix Focus Areas
- pkg/sql/colexec/top/top_test.go[100-136]
- pkg/tests/issues/issue_test.go[1-200]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Null compare copy leaves varlena~~ ☑ 🐞 ≡

Description

strCompare.Copy/arrayCompare.Copy return early on NULL sources after only setting the destination
NULL bitmap, leaving the destination varlena header unchanged. Reused heap slots can then expose
stale bytes (or still panic if later dereferenced) via Vector.GetBytesAt call sites that don't check
NULLs (e.g., Top.getTopValue).

Code

pkg/compare/strcompare.go[R40-43]

if c.isConstNull[vecSrc] || c.vs[vecSrc].GetNulls().Contains(uint64(src)) {
nulls.Add(c.vs[vecDst].GetNulls(), uint64(dst))
return nil
-	} else {
-		nulls.Del(c.vs[vecDst].GetNulls(), uint64(dst))
-		return c.vs[vecDst].Copy(c.vs[vecSrc], dst, src, proc.Mp())
}

Evidence
The NULL fast-path in compare.Copy sets the destination null bit and returns without
clearing/copying data, so any prior varlena header at dst remains. Vector.GetBytesAt always
dereferences the varlena header (no NULL check), and Top.getTopValue uses GetBytesAt for varlen
order columns without checking NULLs, so a NULL row in the heap root can read stale headers/bytes.
pkg/compare/strcompare.go[34-46]
pkg/compare/arraycompare.go[34-46]
pkg/container/vector/vector.go[295-302]
pkg/sql/colexec/top/top.go[332-341]
pkg/sql/colexec/top/top.go[262-274]
pkg/container/vector/vector.go[1115-1137]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`strCompare.Copy` and `arrayCompare.Copy` return early for NULL sources after only setting the destination null bitmap. For varlen vectors this leaves the destination `types.Varlena` header at `dst` unchanged (potentially containing a prior non-NULL row’s offset/len). Later code that calls `GetBytesAt` without checking NULLs (e.g. `Top.getTopValue`) can read stale bytes or still hit an out-of-bounds panic.
### Issue Context
`Vector.Copy` was updated to safely handle NULL sources by clearing destination varlena headers before setting NULL, but the compare-level NULL fast-path bypasses `Vector.Copy`, so the destination header may remain stale in reused slots.
### Fix Focus Areas
- pkg/compare/strcompare.go[34-46]
- pkg/compare/arraycompare.go[34-46]
### Suggested fix
In both `strCompare.Copy` and `arrayCompare.Copy`, when the source row is NULL (including const-null), ensure the destination varlena header is cleared. Options:
1) Call `c.vs[vecDst].Copy(c.vs[vecSrc], dst, src, proc.Mp())` even in the NULL path (now safe because `Vector.Copy` checks NULLs first and clears destination headers), and rely on `Vector.Copy` to set/clear destination NULLs; or
2) Explicitly clear the destination `types.Varlena{}` header for non-fixed-len types before returning.
Keep grouping bitmap handling as-is.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ⓘ The new review experience is currently in Beta. Learn more

mergify · 2026-04-07T14:57:12Z

fix: avoid top panic on null varlen copy

f873eea

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 April 3, 2026 07:17

LeftHandCold had a problem deploying to ci April 3, 2026 07:17 — with GitHub Actions Error

LeftHandCold temporarily deployed to ci April 3, 2026 07:17 — with GitHub Actions Inactive

LeftHandCold had a problem deploying to ci April 3, 2026 07:17 — with GitHub Actions Error

LeftHandCold temporarily deployed to ci April 3, 2026 07:17 — with GitHub Actions Inactive

LeftHandCold had a problem deploying to ci April 3, 2026 07:17 — with GitHub Actions Error

matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Apr 3, 2026

Merge branch '3.0-dev' into fix_avoid_top_panic

0bc64fd

mergify Bot added the kind/bug Something isn't working label Apr 3, 2026

mergify Bot temporarily deployed to ci April 3, 2026 07:18 Inactive

mergify Bot had a problem deploying to ci April 3, 2026 07:18 Failure

mergify Bot temporarily deployed to ci April 3, 2026 07:18 Inactive

LeftHandCold had a problem deploying to ci April 7, 2026 07:19 — with GitHub Actions Failure

LeftHandCold temporarily deployed to ci April 7, 2026 07:19 — with GitHub Actions Inactive

LeftHandCold had a problem deploying to ci April 7, 2026 07:19 — with GitHub Actions Failure

LeftHandCold temporarily deployed to ci April 7, 2026 07:19 — with GitHub Actions Inactive

LeftHandCold temporarily deployed to ci April 7, 2026 08:21 — with GitHub Actions Inactive

LeftHandCold had a problem deploying to ci April 7, 2026 08:21 — with GitHub Actions Failure

LeftHandCold temporarily deployed to ci April 7, 2026 08:21 — with GitHub Actions Inactive

LeftHandCold force-pushed the fix_avoid_top_panic branch from 5e6f147 to 0bc64fd Compare April 7, 2026 10:42

LeftHandCold temporarily deployed to ci April 7, 2026 10:42 — with GitHub Actions Inactive

LeftHandCold had a problem deploying to ci April 7, 2026 10:42 — with GitHub Actions Failure

LeftHandCold temporarily deployed to ci April 7, 2026 10:42 — with GitHub Actions Inactive

Merge branch '3.0-dev' into fix_avoid_top_panic

2a84348

XuPeng-SH approved these changes Apr 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: avoid top panic on null varlen copy#24049

fix: avoid top panic on null varlen copy#24049
mergify[bot] merged 3 commits intomatrixorigin:3.0-devfrom
LeftHandCold:fix_avoid_top_panic

LeftHandCold commented Apr 3, 2026

Uh oh!

qodo-code-review Bot commented Apr 3, 2026

Uh oh!

qodo-code-review Bot commented Apr 3, 2026 •

edited

Loading

Uh oh!

mergify Bot commented Apr 7, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

LeftHandCold commented Apr 3, 2026

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

Uh oh!

qodo-code-review Bot commented Apr 3, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review Bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

mergify Bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

qodo-code-review Bot commented Apr 3, 2026 •

edited

Loading

mergify Bot commented Apr 7, 2026 •

edited

Loading