Skip to content

Conversation

@aunjgr
Copy link
Contributor

@aunjgr aunjgr 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 #22508

What this PR does / why we need it:

empty index should return empty result


PR Type

Bug fix


Description

  • Return empty result instead of error when vector index is empty

  • Support multiple batches in search results processing

  • Update test expectations to reflect new behavior

  • Add integration test case for empty index queries


Diagram Walkthrough

flowchart LR
  A["Search Query"] --> B["Execute SQL"]
  B --> C{"Batches Empty?"}
  C -->|Old| D["Return Error"]
  C -->|New| E["Return Empty Result"]
  B --> F["Process Batches"]
  F --> G["Return Results"]
Loading

File Walkthrough

Relevant files
Bug fix
search.go
Handle empty batches gracefully in search                               

pkg/vectorindex/ivfflat/search.go

  • Removed error return when result batches are empty
  • Changed to return empty slices instead of error
  • Modified loop to iterate over all batches instead of just first batch
  • Moved empty check after batch processing initialization
+12/-10 
Tests
search_test.go
Update tests for empty index behavior                                       

pkg/vectorindex/ivfflat/search_test.go

  • Renamed mock function from mock_runSql_streaming to mock_runSql
  • Renamed mock error function from mock_runSql_streaming_parser_error to
    mock_runSql_parser_error
  • Updated test expectation from require.NotNil to require.Nil for empty
    index case
  • Updated function references in test cases
+5/-5     
vector_ivf.sql
Add integration test for empty index query                             

test/distributed/cases/vector/vector_ivf.sql

  • Added SELECT query test case for empty vector index search
  • Query searches on empty table with L2_distance filter
  • Tests vector search with status filter returning no results
  • Demonstrates expected empty result behavior
+13/-0   
vector_ivf.result
Add expected test results for empty query                               

test/distributed/cases/vector/vector_ivf.result

  • Added expected result output for empty vector index query test
  • Shows empty result set with column headers
  • Corresponds to new SQL test case for empty searches
+13/-0   

@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
🟢
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 <!-- /create_ticket --create_ticket=true -->

</details></td></tr>
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

@mergify mergify bot added the kind/bug Something isn't working label Oct 22, 2025
@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
Prevent resource leak with defer

Use a defer statement to ensure res.Close() is always called after runSql
returns a result, preventing a potential resource leak on early returns.

pkg/vectorindex/ivfflat/search.go [193-213]

+	res, err := runSql(proc, sql)
+	if err != nil {
+		return
+	}
+	defer res.Close()
+
+	if len(rt.BackgroundQueries) > 0 {
+		rt.BackgroundQueries[0] = res.LogicalPlan
+	}
+
 	distances = make([]float64, 0, rt.Limit)
 	resid := make([]any, 0, rt.Limit)
 
 	if len(res.Batches) == 0 {
 		return resid, distances, nil
 	}
 
 	for _, bat := range res.Batches {
 		for i := 0; i < bat.RowCount(); i++ {
 			pk := vector.GetAny(bat.Vecs[0], i, true)
 			resid = append(resid, pk)
 
 			dist := vector.GetFixedAtNoTypeCheck[float64](bat.Vecs[1], i)
 			dist = metric.DistanceTransformIvfflat(dist, idxcfg.OpType, metric.MetricType(idxcfg.Ivfflat.Metric))
 			distances = append(distances, dist)
 		}
 	}
 
-	res.Close()
-
 	return resid, distances, nil
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a resource leak introduced in the PR due to an early return path where res.Close() is not called, and it proposes the idiomatic Go solution using defer.

Medium
  • Update

@mergify mergify bot added the queued label Oct 24, 2025
@mergify mergify bot merged commit d8400fc into matrixorigin:main Oct 24, 2025
19 checks passed
@mergify mergify bot removed the queued label Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working Review effort 2/5 size/S Denotes a PR that changes [10,99] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants