Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

push down ORDER BY LIMIT clause on IVFFlat index to table_scan #22613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aunjgr wants to merge 21 commits into matrixorigin:main
base: main
Choose a base branch
Loading
from aunjgr:vecidx

Conversation

Copy link
Contributor

@aunjgr aunjgr commented Oct 13, 2025
edited
Loading

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #22508

What this PR does / why we need it:

  • Put all searching logic into background query
  • Push down ORDER BY LIMIT clause on index table to scan node and implement it in Reader
  • Make analyze info of this background query visible
  • Update dependencies to newest (uSearch, SimSIMD, ...)

In particular query, number of copied rows from cache to pipeline is reduced from 13560 to 270, and throughput is improved by 10 times.


PR Type

Enhancement, Tests


Description

  • Push down ORDER BY LIMIT clause on vector index to table scan for IVFFlat

  • Update mock interfaces to support SetLimit method

  • Refactor IVFFlat search to use SQL-based distance computation

  • Update dependencies (StringZilla, SimSIMD)


Diagram Walkthrough

flowchart LR
 A["Query Builder"] -- "pushdown optimization" --> B["Table Scan Node"]
 B -- "ORDER BY + LIMIT" --> C["IVFFlat Search"]
 C -- "SQL with ORDER BY LIMIT" --> D["Database Entries"]
 D -- "sorted results" --> E["Final Output"]
Loading

File Walkthrough

Relevant files
Enhancement
17 files
pushdown.go
Add vector index ORDER BY LIMIT pushdown logic
+61/-0
query_builder.go
Integrate pushdown optimization into query building
+15/-0
compile.go
Update compilation to handle ORDER BY LIMIT pushdown
+99/-98
scope.go
Set limit on data source readers
+2/-0
types.go
Add Limit field to Source struct
+1/-0
search.go
Refactor search to use SQL-based distance computation
+31/-135
types.go
Add metric type to distance function name mapping
+8/-0
types.go
Add BlockReadTopOp struct for limit operations
+4/-0
local_disttae_datasource.go
Implement SetLimit method for local data source
+7/-3
types.go
Add SetLimit to DataSource and Reader interfaces
+2/-0
reader.go
Implement SetLimit for various reader types
+19/-0
datasource.go
Add SetLimit stub for remote data source
+5/-2
read.go
Add orderByLimit parameter to BlockDataRead function
+1/-0
table_meta_reader.go
Add SetLimit stub for table meta reader
+3/-0
txn_table_delegate.go
Add SetLimit stub for sharding local reader
+3/-0
table_reader.go
Add SetLimit stub for memory engine reader
+3/-0
backup.go
Add SetLimit panic stub for backup delta source
+11/-8
Tests
1 files
engine_mock.go
Update mock interfaces with SetLimit and signature fixes
+46/-20
Dependencies
3 files
go.mod
Update golang.org/x dependencies to newer versions
+9/-9
go.sum
Update checksums for golang.org/x dependencies
+20/-20
Makefile
Update StringZilla and SimSIMD library versions
+2/-2

@matrix-meow matrix-meow added the size/L Denotes a PR that changes [500,999] lines label Oct 13, 2025
Copy link

qodo-merge-pro bot commented Oct 13, 2025
edited
Loading

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link

qodo-merge-pro bot commented Oct 13, 2025
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion Impact
Possible issue
Refine order-by handling logic

Refine the handleOrderBy logic to avoid disabling Zone Map optimization for
regular ORDER BY ... LIMIT queries. The optimization should only be skipped for
vector index scans, which can be identified by checking if the ORDER BY
expression is a function call.

pkg/vm/engine/disttae/local_disttae_datasource.go [390-409]

 func (ls *LocalDisttaeDataSource) handleOrderBy() {
 	// for ordered scan, sort blocklist by zonemap info, and then filter by zonemap
-	if len(ls.OrderBy) > 0 && ls.Limit == 0 {
+	if len(ls.OrderBy) > 0 {
+		// if order by expression is a function call, we can't use zonemap to sort blocks.
+		// This is the case for vector index scans.
+		if ls.OrderBy[0].Expr.GetF() != nil {
+			return
+		}
+
 		if !ls.sorted {
 			ls.desc = ls.OrderBy[0].Flag&plan.OrderBySpec_DESC != 0
 			ls.getBlockZMs()
 			ls.sortBlockList()
 			ls.sorted = true
 		}
 		i := ls.rangesCursor
 		sliceLen := ls.rangeSlice.Len()
 		for i < sliceLen {
 			if ls.needReadBlkByZM(i) {
 				break
 			}
 			i++
 		}
 		ls.rangesCursor = i
 	}
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a performance regression for ORDER BY ... LIMIT queries caused by the overly broad condition ls.Limit == 0, and provides a more precise fix to disable Zone Map optimization only for vector index scans.

Medium
General
(削除) Correct block number stats estimation (削除ここまで)
Suggestion Impact:The commit removed the previous code that modified Stats.BlockNum based on the limit and no longer adjusts BlockNum after pushdown.

code diff:

 		// if there is a limit, outcnt is limit number
-		scanNode.Stats.Outcnt = float64(limitVal)
-		if scanNode.Stats.Selectivity < 0.5 {
-			newblockNum := int32(limitVal / 2)
-			if newblockNum < scanNode.Stats.BlockNum {
-				scanNode.Stats.BlockNum = newblockNum
-			}
-		} else {
-			scanNode.Stats.BlockNum = 1
-		}
+		scanNode.Stats.Outcnt = float64(scanNode.Stats.BlockNum) * float64(limitVal)
 		scanNode.Stats.Cost = float64(scanNode.Stats.BlockNum * objectio.BlockMaxRows)
 	}

Remove the overly optimistic logic for updating scanNode.Stats.BlockNum after
pushing down a LIMIT for a vector index scan. The number of blocks to scan is
determined by the number of probes, not the limit value, so it's safer to leave
the block number statistic as is.

pkg/sql/plan/pushdown.go [646-653]

-		if scanNode.Stats.Selectivity < 0.5 {
-			newblockNum := int32(limitVal / 2)
-			if newblockNum < scanNode.Stats.BlockNum {
-				scanNode.Stats.BlockNum = newblockNum
-			}
-		} else {
-			scanNode.Stats.BlockNum = 1
-		}
+		// After pushdown, the number of blocks to be scanned is still determined by the number of probes,
+		// so we don't need to adjust the BlockNum.
+		// The Outcnt is already updated to reflect the limit.

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that the heuristic for updating scanNode.Stats.BlockNum is overly optimistic and could lead to suboptimal query plans, proposing a safer approach by not modifying it.

Medium
Security
(削除) Remove debug print and prevent injection (削除ここまで)
Suggestion Impact:The commit commented out the debug print line fmt.Println("IVFFlat SQL: ", sql), effectively removing the debug output as suggested.

code diff:

-	fmt.Println("IVFFlat SQL: ", sql)
+	//fmt.Println("IVFFlat SQL: ", sql)
 	//os.Stderr.WriteString(sql)

Remove the fmt.Println debugging statement from the vector search query
construction. Using fmt.Sprintf for SQL queries is also flagged as a potential
security risk.

pkg/vectorindex/ivfflat/search.go [188-202]

 	sql := fmt.Sprintf(
 		"SELECT `%s`, %s(`%s`, '%s') as vec_dist FROM `%s`.`%s` WHERE `%s` = %d AND `%s` IN (%s) ORDER BY vec_dist LIMIT %d",
 		catalog.SystemSI_IVFFLAT_TblCol_Entries_pk,
 		metric.MetricTypeToDistFuncName[metric.MetricType(idxcfg.Ivfflat.Metric)],
 		catalog.SystemSI_IVFFLAT_TblCol_Entries_entry,
 		types.ArrayToString(query),
 		tblcfg.DbName, tblcfg.EntriesTable,
 		catalog.SystemSI_IVFFLAT_TblCol_Entries_version,
 		idx.Version,
 		catalog.SystemSI_IVFFLAT_TblCol_Entries_id,
 		instr,
 		rt.Limit,
 	)
 
-	fmt.Println("IVFFlat SQL: ", sql)
-

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a debugging artifact (fmt.Println) that should be removed from production code, which is a valid but minor code quality issue.

Low
  • Update

@matrix-meow matrix-meow added size/L Denotes a PR that changes [500,999] lines and removed size/XXL Denotes a PR that changes 2000+ lines labels Oct 17, 2025
@matrix-meow matrix-meow added size/XL Denotes a PR that changes [1000, 1999] lines and removed size/L Denotes a PR that changes [500,999] lines labels Oct 17, 2025
@aunjgr aunjgr requested a review from cpegeric October 17, 2025 15:00
Copy link
Contributor

@cpegeric cpegeric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only question here is single thread can be faster than multi-thread when find centroids?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpegeric cpegeric cpegeric approved these changes

@reusee reusee reusee approved these changes

@LeftHandCold LeftHandCold LeftHandCold approved these changes

@gouhongshen gouhongshen gouhongshen approved these changes

@XuPeng-SH XuPeng-SH XuPeng-SH approved these changes

@ouyuanning ouyuanning ouyuanning approved these changes

@jiangxinmeng1 jiangxinmeng1 jiangxinmeng1 approved these changes

@daviszhen daviszhen daviszhen approved these changes

@fengttt fengttt Awaiting requested review from fengttt fengttt is a code owner

@zhangxu19830126 zhangxu19830126 Awaiting requested review from zhangxu19830126 zhangxu19830126 is a code owner

@heni02 heni02 Awaiting requested review from heni02 heni02 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

kind/enhancement kind/feature Review effort 3/5 size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /