Skip to content

Conversation

@LeftHandCold
Copy link
Contributor

@LeftHandCold LeftHandCold commented Oct 22, 2025

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 #22634

What this PR does / why we need it:

test not space


PR Type

Bug fix, Enhancement


Description

  • Revert ORDER BY LIMIT pushdown optimization on IVFFlat index

  • Refactor IVFFlat search to use streaming SQL with multi-threaded processing

  • Rename plan node parameters from nodes to ns for consistency

  • Remove BlockOrderBy and BlockLimit fields from query plan

  • Update dependencies and clean up unused imports


Diagram Walkthrough

flowchart LR
  A["IVFFlat Search"] -->|"Streaming SQL"| B["Multi-threaded Processing"]
  B -->|"Safe Heap"| C["Result Aggregation"]
  D["Plan Node"] -->|"Remove BlockOrderBy/BlockLimit"| E["Simplified Plan"]
  F["Dependencies"] -->|"Update versions"| G["Aligned packages"]
Loading

File Walkthrough

Relevant files
Refactoring
2 files
compile.go
Rename nodes parameter to ns throughout function                 
+98/-100
sql_executor.go
Simplify plan building by removing statement type switch 
+1/-23   
Enhancement
3 files
search.go
Implement streaming SQL with multi-threaded search             
+197/-48
ivf_search.go
Remove background queries tracking from IVF search             
+5/-13   
types.go
Remove BackgroundQueries field from RuntimeConfig               
+2/-4     
Tests
4 files
search_test.go
Add streaming mock functions and cancel test case               
+57/-2   
ivf_search_test.go
Add generic type parameters to vector operations                 
+7/-6     
pushdown.result
Update expected output removing table prefix from sort keys
+3/-3     
top.result
Update expected output removing table prefix from sort keys
+3/-3     
Bug fix
6 files
constant_fold.go
Remove BlockLimit and BlockOrderBy constant folding           
+1/-17   
explain_node.go
Remove BlockOrderBy and BlockLimit explain logic                 
+1/-37   
local_disttae_datasource.go
Remove Limit field and condition check from OrderBy handling
+3/-3     
db_test.go
Remove extra nil parameter from BlockDataReadInner calls 
+10/-10 
stats.go
Remove NodeType condition from limit calculation                 
+1/-1     
plan.proto
Remove BlockOrderBy and BlockLimit message fields               
+4/-7     
Formatting
5 files
read.go
Remove unused imports and reorganize dependencies               
+4/-87   
backup.go
Reorganize imports and add zap logging import                       
+8/-7     
reader.go
Add zap logging import                                                                     
+2/-50   
datasource.go
Reorganize imports for consistency                                             
+3/-3     
types.go
Add blank line in DataSource interface                                     
+1/-1     
Dependencies
3 files
Makefile
Downgrade USearch and related library versions                     
+4/-4     
go.mod
Update Go dependencies and reorganize require blocks         
+67/-64 
go.sum
Update checksums for Go dependencies                                         
+22/-22 
Configuration changes
1 files
.golangci.yml
Enable SA1019 staticcheck rule                                                     
+1/-1     
Additional files
13 files
engine_mock.go +0/-12   
types.go +0/-10   
plan.pb.go +985/-1113
scope.go +0/-4     
types.go +0/-3     
deepcopy.go +0/-6     
pushdown.go +0/-53   
query_builder.go +0/-29   
index.go +0/-26   
types.go +0/-18   
table_meta_reader.go +0/-3     
txn_table_delegate.go +0/-3     
table_reader.go +0/-3     

@matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label Oct 22, 2025
@mergify mergify bot added the kind/test-ci label Oct 22, 2025
@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Oct 22, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Resource leak risk

Description: Streaming SQL consumer may leak memory if producer errors are not fully drained or
channels are mishandled; ensure error and result channels are always drained/closed and
context cancellations are correctly propagated to avoid unbounded buffering or goroutine
leaks.
search.go [214-341]

Referred Code
	nthread int64,
) (keys any, distances []float64, err error) {

	stream_chan := make(chan executor.Result, nthread)
	error_chan := make(chan error, nthread)

	distfn, err := metric.ResolveDistanceFn[T](metric.MetricType(idxcfg.Ivfflat.Metric))
	if err != nil {
		return nil, nil, err
	}

	centroids_ids, err := idx.findCentroids(proc, query, distfn, idxcfg, rt.Probe, nthread)
	if err != nil {
		return nil, nil, err
	}

	var instr string
	for i, c := range centroids_ids {
		if i > 0 {
			instr += ","
		}


 ... (clipped 107 lines)
DoS via drain

Description: Results from stream channel are drained and closed without a bounded timeout after
cancellation, which could allow a malicious or buggy producer to block shutdown leading to
denial of service.
search.go [318-323]

Referred Code
if !gStreamClosed.Load() {
	for res := range stream_chan {
		res.Close()
	}
}
Ticket Compliance
🟡
🎫 #22634
🔴 Ensure that created database snapshots are not garbage collected (GC) while referenced by
a snapshot.
Ensure that created table snapshots are not garbage collected (GC) while referenced by a
snapshot.
Provide tests or CI coverage that validates snapshot retention behavior.
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

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Oct 22, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid reusing protobuf field tags

Revert the protobuf field tag for SourceStep from 32 back to 27. Reusing a tag
for a field of a different type breaks backward compatibility and can cause data
corruption.

pkg/pb/plan/plan.pb.go [6351-6358]

 	TblFuncExprList []*Expr       `protobuf:"bytes,26,rep,name=tbl_func_expr_list,json=tblFuncExprList,proto3" json:"tbl_func_expr_list,omitempty"`
-+	ClusterTable    *ClusterTable `protobuf:"bytes,28,opt,name=cluster_table,json=clusterTable,proto3" json:"cluster_table,omitempty"`
-+	NotCacheable    bool          `protobuf:"varint,29,opt,name=not_cacheable,json=notCacheable,proto3" json:"not_cacheable,omitempty"`
-+	InsertCtx       *InsertCtx    `protobuf:"bytes,30,opt,name=insert_ctx,json=insertCtx,proto3" json:"insert_ctx,omitempty"`
-+	ReplaceCtx      *ReplaceCtx   `protobuf:"bytes,31,opt,name=replace_ctx,json=replaceCtx,proto3" json:"replace_ctx,omitempty"`
 	// used to connect two plans[steps]
--	SourceStep   []int32        `protobuf:"varint,27,rep,packed,name=source_step,json=sourceStep,proto3" json:"source_step,omitempty"`
--	ClusterTable *ClusterTable  `protobuf:"bytes,28,opt,name=cluster_table,json=clusterTable,proto3" json:"cluster_table,omitempty"`
--	NotCacheable bool           `protobuf:"varint,29,opt,name=not_cacheable,json=notCacheable,proto3" json:"not_cacheable,omitempty"`
--	InsertCtx    *InsertCtx     `protobuf:"bytes,30,opt,name=insert_ctx,json=insertCtx,proto3" json:"insert_ctx,omitempty"`
--	ReplaceCtx   *ReplaceCtx    `protobuf:"bytes,31,opt,name=replace_ctx,json=replaceCtx,proto3" json:"replace_ctx,omitempty"`
--	BlockOrderBy []*OrderBySpec `protobuf:"bytes,32,rep,name=block_order_by,json=blockOrderBy,proto3" json:"block_order_by,omitempty"`
--	BlockLimit   *Expr          `protobuf:"bytes,33,opt,name=block_limit,json=blockLimit,proto3" json:"block_limit,omitempty"`
--	PreInsertCtx *PreInsertCtx  `protobuf:"bytes,34,opt,name=pre_insert_ctx,json=preInsertCtx,proto3" json:"pre_insert_ctx,omitempty"`
-+	SourceStep   []int32       `protobuf:"varint,32,rep,packed,name=source_step,json=sourceStep,proto3" json:"source_step,omitempty"`
-+	PreInsertCtx *PreInsertCtx `protobuf:"bytes,34,opt,name=pre_insert_ctx,json=preInsertCtx,proto3" json:"pre_insert_ctx,omitempty"`
+	SourceStep      []int32       `protobuf:"varint,27,rep,packed,name=source_step,json=sourceStep,proto3" json:"source_step,omitempty"`
+	ClusterTable    *ClusterTable `protobuf:"bytes,28,opt,name=cluster_table,json=clusterTable,proto3" json:"cluster_table,omitempty"`
+	NotCacheable    bool          `protobuf:"varint,29,opt,name=not_cacheable,json=notCacheable,proto3" json:"not_cacheable,omitempty"`
+	InsertCtx       *InsertCtx    `protobuf:"bytes,30,opt,name=insert_ctx,json=insertCtx,proto3" json:"insert_ctx,omitempty"`
+	ReplaceCtx      *ReplaceCtx   `protobuf:"bytes,31,opt,name=replace_ctx,json=replaceCtx,proto3" json:"replace_ctx,omitempty"`
+	PreInsertCtx    *PreInsertCtx `protobuf:"bytes,34,opt,name=pre_insert_ctx,json=preInsertCtx,proto3" json:"pre_insert_ctx,omitempty"`
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical backward-compatibility issue where a protobuf field tag (32) is reused for a field (SourceStep) with a different type, which can lead to data corruption or panics.

High
Drain error channel to prevent deadlocks

Drain the error_chan completely to handle all reported errors and prevent
potential deadlocks from blocked goroutines.

pkg/vectorindex/ivfflat/search.go [326-330]

 	// fetch potential remaining errors from error_chan
 	// if error is not nil, fast return
-	select {
-	case err = <-error_chan:
+	for len(error_chan) > 0 {
+		if err == nil {
+			err = <-error_chan
+		} else {
+			<-error_chan
+		}
+	}
+	if err != nil {
 		return
-	default:
 	}

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that multiple errors can be sent to error_chan, but only one is read, which could hide other errors and potentially cause goroutines to block if the channel fills up.

Medium
Add nil check for search vector

Add a check in runIvfSearchVector to ensure the retrieved vector fa is not empty
before calling the search function.

pkg/sql/colexec/table_function/ivf_search.go [231-252]

 func runIvfSearchVector[T types.RealNumbers](u *ivfSearchState, proc *process.Process, faVec *vector.Vector, nthRow int) (err error) {
 	if faVec.IsNull(uint64(nthRow)) {
 		return nil
 	}
 	fa := vector.GetArrayAt[T](faVec, nthRow)
+	if len(fa) == 0 {
+		return nil
+	}
 	algo, err := u.newIvfAlgoFn(u.idxcfg, u.tblcfg)
 	if err != nil {
 		return err
 	}
 	key := fmt.Sprintf("%s:%d", u.tblcfg.IndexTable, u.idxcfg.Ivfflat.Version)
 	u.keys, u.distances, err = veccache.Cache.Search(proc, key, algo, fa, vectorindex.RuntimeConfig{Limit: uint(u.limit), Probe: uint(u.tblcfg.Nprobe)})
 	if err != nil {
 		return err
 	}
 
 	return nil
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out a missing validation for an empty vector slice, which could lead to wasted work or unexpected behavior in the downstream Search function.

Low
High-level
PR content mismatches its description

The PR's code changes, which involve refactoring vector index search and the
query plan, are inconsistent with its linked ticket that addresses a bug in
database snapshots. This mismatch obscures the PR's purpose.

Examples:

Solution Walkthrough:

Before:

// PR Description
PR for: "Issue #22634: Snapshots do not support databases or tables"

// Code Changes
- Refactor IVFFlat search to use streaming and multi-threading.
- Remove `BlockOrderBy` and `BlockLimit` from the query plan.
- Update various dependencies.
- ... and other changes unrelated to snapshots.

After:

// PR Description
PR for: "Issue #XXXXX: Revert ORDER BY LIMIT pushdown for IVFFlat and refactor search"

// Code Changes
- Refactor IVFFlat search to use streaming and multi-threading.
- Remove `BlockOrderBy` and `BlockLimit` from the query plan.
- Update various dependencies.
- ... (code changes remain the same, but now match the description).

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical discrepancy between the linked ticket (about snapshots) and the actual code changes (IVFFlat search refactor), which severely hinders understanding the PR's intent and traceability.

High
  • Update

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants