fix persisted without deal with distVec by iamlinjunhong · Pull Request #23813 · matrixorigin/matrixone

iamlinjunhong · 2026-03-05T09:27:06Z

What type of PR is this?

Which issue(s) this PR fixes:

issue #23781

What this PR does / why we need it:

fix persisted without deal with distVec

qodo-code-review · 2026-03-05T09:27:27Z

Review Summary by Qodo

Fix distance vector handling in IVFFLAT TopN pushdown with persisted data

🐞 Bug fix

Walkthroughs

Description

• Extract output batch filling logic into reusable function
• Handle distance vector properly when TopN pushdown applied
• Fix panic when scanning persisted IVFFLAT entries with in-memory deltas
• Add regression test for mixed persisted and in-memory data scenario

Diagram

flowchart LR
  A["BlockDataReadInner<br/>with TopN pushdown"] -->|extract common logic| B["fillOutputBatchBySelectedRows<br/>new function"]
  C["Pre-filter rows path"] -->|call| B
  D["No pre-filter with TopN path"] -->|call| B
  B -->|properly handle| E["Distance vector<br/>appending"]

File Changes

1. pkg/vm/engine/tae/blockio/read.go 🐞 Bug fix +113/-45

Refactor and fix TopN pushdown distance vector handling

• Extract 58 lines of output batch filling logic into new fillOutputBatchBySelectedRows function
• Add handling for distance vector appending when orderByLimit is not nil
• Add new code path for TopN pushdown on live rows when no pre-filter rows exist
• Replace duplicated logic in BlockDataReadInner with calls to extracted function

pkg/vm/engine/tae/blockio/read.go

2. test/distributed/cases/vector/vector_ivfflat_entries_panic_repro.sql 🧪 Tests +153/-0

Add IVFFLAT entries panic reproduction test case

• Create test case reproducing panic with IVFFLAT entries on mixed persisted and in-memory data
• Insert 10000 baseline rows with embedding [0.1,0.2,0.3,0.4] and flush to persist
• Insert 4000 additional rows with embedding [0.5,0.5,0.5,0.5] in-memory after checkpoint
• Execute TopN query with LIMIT 20000 on entries table to trigger the bug scenario

test/distributed/cases/vector/vector_ivfflat_entries_panic_repro.sql

3. test/distributed/cases/vector/vector_ivfflat_entries_panic_repro.result 🧪 Tests +156/-0

Add expected test results for IVFFLAT panic test
• Expected output for IVFFLAT entries panic reproduction test
• Shows successful execution with correct result count of 4000
• Includes flush and checkpoint operations output
test/distributed/cases/vector/vector_ivfflat_entries_panic_repro.result

qodo-code-review · 2026-03-05T09:27:28Z

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. ~~TopOp type mismatch~~ ☑ 🐞 Bug ✓ Correctness

Description

fillOutputBatchBySelectedRows takes *objectio.BlockReadTopOp, but BlockDataReadInner works with
*objectio.IndexReaderTopOp and passes that value into the helper; this will not type-check unless
BlockReadTopOp exists as a compatible alias (it does not appear in objectio/types.go). This is
likely a hard compile failure / broken build.

Code

pkg/vm/engine/tae/blockio/read.go[R524-526]
+	selectRows []int64,
+	orderByLimit *objectio.BlockReadTopOp,
+	dists []float64,

Evidence
The helper signature requires a different type than the caller has, and objectio defines
IndexReaderTopOp (but the helper references BlockReadTopOp).
pkg/vm/engine/tae/blockio/read.go[518-528]
pkg/vm/engine/tae/blockio/read.go[579-589]
pkg/vm/engine/tae/blockio/read.go[636-646]
pkg/objectio/types.go[86-99]

Agent prompt

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

## Issue description
`fillOutputBatchBySelectedRows` currently takes `*objectio.BlockReadTopOp`, but callers and other TopN code paths use `*objectio.IndexReaderTopOp`. This creates a type mismatch and likely breaks compilation.
### Issue Context
`HandleOrderByLimitOnIVFFlatIndex` and `BlockDataReadInner` both use `*objectio.IndexReaderTopOp`.
### Fix Focus Areas
- pkg/vm/engine/tae/blockio/read.go[518-528]
- pkg/vm/engine/tae/blockio/read.go[636-646]
- pkg/vm/engine/tae/blockio/read.go[704-714]

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

2. Dist vec append unchecked 🐞 Bug ⛯ Reliability

Description

fillOutputBatchBySelectedRows ignores the error return from vector.AppendFixedList when populating
the distance vector. Under memory pressure or append failures, this can silently produce an
incorrect/partial batch rather than returning an error.

Code

pkg/vm/engine/tae/blockio/read.go[R565-572]

+	if orderByLimit != nil {
+		if len(outputBat.Vecs) == len(columns) {
+			distVec := vector.NewVec(types.T_float64.ToType())
+			vector.AppendFixedList(distVec, dists, nil, mp)
+			outputBat.Vecs = append(outputBat.Vecs, distVec)
+		} else {
+			vector.AppendFixedList(outputBat.Vecs[len(outputBat.Vecs)-1], dists, nil, mp)
+		}

Evidence
AppendFixedList explicitly returns an error, but the new helper drops it in both branches, so
failures won’t propagate to the caller.
pkg/vm/engine/tae/blockio/read.go[565-572]
pkg/container/vector/vector.go[3234-3245]

Agent prompt

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

## Issue description
`vector.AppendFixedList` returns an `error`, but the new helper ignores it. This can hide OOM/append failures and lead to incorrect results.
### Issue Context
The in-mem TopN path already checks this error; persisted path should be consistent.
### Fix Focus Areas
- pkg/vm/engine/tae/blockio/read.go[565-572]

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

3. Wasted delete conversion 🐞 Bug ➹ Performance

Description

In the new persisted TopN early-return path, BlockDataReadInner still converts deleteMask into
deletedRows before checking orderByLimit and returning. This conversion is unused in the TopN
branch, creating avoidable CPU/alloc overhead on TopN scans with tombstones.

Code

pkg/vm/engine/tae/blockio/read.go[R675-715]

+	// No pre-filter rows, but vector TopN pushdown is requested:
+	// apply TopN on live rows (exclude tombstones first), then materialize selected rows.
+	if orderByLimit != nil {
+		vecColPos := orderByLimit.ColPos
+		if phyAddrColumnPos >= 0 && vecColPos > int32(phyAddrColumnPos) {
+			vecColPos--
+		}
+		vecCol := &cacheVectors[vecColPos]
+
+		var topInputRows []int64
+		if !deleteMask.IsEmpty() {
+			capHint := vecCol.Length() - deleteMask.Count()
+			if capHint < 0 {
+				capHint = 0
+			}
+			topInputRows = make([]int64, 0, capHint)
+			for i := 0; i < vecCol.Length(); i++ {
+				if !deleteMask.Contains(uint64(i)) {
+					topInputRows = append(topInputRows, int64(i))
+				}
+			}
+		}
+
+		var dists []float64
+		selectRows, dists, err = HandleOrderByLimitOnIVFFlatIndex(ctx, topInputRows, vecCol, orderByLimit)
+		if err != nil {
+			return err
+		}
+
+		return fillOutputBatchBySelectedRows(
+			info,
+			columns,
+			phyAddrColumnPos,
+			outputBat,
+			cacheVectors,
+			selectRows,
+			orderByLimit,
+			dists,
+			mp,
+		)
+	}

Evidence
deletedRows is built from the bitmap, but when orderByLimit != nil the function returns from the new
TopN branch before reaching the code path that Shrinks vectors using deletedRows.
pkg/vm/engine/tae/blockio/read.go[664-673]
pkg/vm/engine/tae/blockio/read.go[675-715]

Agent prompt

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

## Issue description
`deletedRows = deleteMask.ToI64Array(...)` is computed even when `orderByLimit != nil` and the function returns early from the new TopN branch, so the conversion work is wasted.
### Issue Context
The TopN branch already handles deletes by building `topInputRows` excluding tombstones; it does not call `Shrink(deletedRows, ...)`.
### Fix Focus Areas
- pkg/vm/engine/tae/blockio/read.go[664-715]

ⓘ 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

pkg/vm/engine/tae/blockio/read.go

mergify · 2026-03-11T11:54:57Z

Merge Queue Status

✅ Entered queue — 2026-03-11 11:54 UTC · Rule: main
✅ Checks passed · in-place
✅ Merged — 2026-03-11 12:52 UTC · at 0a0007e52edaf97bd0c452bbefe15f5a05d16645

This pull request spent 57 minutes 32 seconds in the queue, including 57 minutes 17 seconds running CI.

Required conditions to merge

iamlinjunhong requested review from LeftHandCold, XuPeng-SH and heni02 as code owners March 5, 2026 09:27

iamlinjunhong had a problem deploying to ci March 5, 2026 09:27 — with GitHub Actions Failure

iamlinjunhong had a problem deploying to ci March 5, 2026 09:27 — with GitHub Actions Error

iamlinjunhong had a problem deploying to ci March 5, 2026 09:27 — with GitHub Actions Failure

iamlinjunhong temporarily deployed to ci March 5, 2026 09:27 — with GitHub Actions Inactive

iamlinjunhong had a problem deploying to ci March 5, 2026 09:27 — with GitHub Actions Failure

iamlinjunhong temporarily deployed to ci March 5, 2026 09:27 — with GitHub Actions Inactive

iamlinjunhong had a problem deploying to ci March 5, 2026 09:27 — with GitHub Actions Failure

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

qodo-code-review bot reviewed Mar 5, 2026

View reviewed changes

pkg/vm/engine/tae/blockio/read.go Show resolved Hide resolved

pkg/vm/engine/tae/blockio/read.go Show resolved Hide resolved

iamlinjunhong temporarily deployed to ci March 5, 2026 09:46 — with GitHub Actions Inactive

iamlinjunhong had a problem deploying to ci March 5, 2026 09:46 — with GitHub Actions Failure

iamlinjunhong temporarily deployed to ci March 5, 2026 09:46 — with GitHub Actions Inactive

iamlinjunhong had a problem deploying to ci March 5, 2026 10:54 — with GitHub Actions Error

iamlinjunhong temporarily deployed to ci March 6, 2026 03:08 — with GitHub Actions Inactive

iamlinjunhong had a problem deploying to ci March 6, 2026 03:08 — with GitHub Actions Failure

iamlinjunhong temporarily deployed to ci March 6, 2026 03:08 — with GitHub Actions Inactive

iamlinjunhong force-pushed the m-d-p branch from 3251c0f to bca2974 Compare March 10, 2026 09:25

iamlinjunhong temporarily deployed to ci March 10, 2026 09:26 — with GitHub Actions Inactive

iamlinjunhong and others added 10 commits March 11, 2026 18:58

fix persisted without deal with distVec

b5da15a

fix

194ab20

ut

94ee719

fix

9817137

ut

cae49d0

ut

a619d90

ut

2bc8405

fix

f80eaa4

fix

d5612c1

Merge branch 'main' into m-d-p

0a0007e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix persisted without deal with distVec#23813

fix persisted without deal with distVec#23813
mergify[bot] merged 10 commits intomatrixorigin:mainfrom
iamlinjunhong:m-d-p

iamlinjunhong commented Mar 5, 2026

Uh oh!

qodo-code-review bot commented Mar 5, 2026

Uh oh!

qodo-code-review bot commented Mar 5, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Mar 11, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

iamlinjunhong commented Mar 5, 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 Mar 5, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

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

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Mar 11, 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 Mar 5, 2026 •

edited

Loading

mergify bot commented Mar 11, 2026 •

edited

Loading