Support dynamic edge size parameter for Search API#3480
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an int32 Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Handler as gRPC Handler
participant Service as NGT Service
participant Core as NGT Core
participant CAPI as C API
Client->>Handler: Search(req { vec, config.edge_size })
Handler->>Handler: extract config.edge_size
Handler->>Service: Search(ctx, vec, size, eps, radius, edgeSize)
Service->>Core: Search(ctx, vec, size, eps, radius, edgeSize)
Core->>CAPI: ngt_search_index_as_float(..., edgeSize)
CAPI-->>Core: results
Core-->>Service: []SearchResult
Service-->>Handler: SearchResponse
Handler-->>Client: SearchResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[WARNING:INTCFG] Changes in |
github-actions
Bot
added
area/manager/index
area/filter/ingress
area/filter/egress
area/internal
area/makefile
area/agent/sidecar
area/gateway/lb
area/gateway/filter
area/agent/core/ngt
area/gateway/mirror
area/index/job/correction
area/index/job/creation
area/index/job/save
area/index/job/readreplica
labels
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
Bot
added
actions/backport/release/v1.7
actions/build/amd64
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3480 +/- ##
==========================================
- Coverage 26.77% 26.04% -0.74%
==========================================
Files 527 569 +42
Lines 48959 50325 +1366
==========================================
- Hits 13110 13106 -4
- Misses 34874 36247 +1373
+ Partials 975 972 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying vald with
|
| Latest commit: |
73be3fd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://84f62583.vald.pages.dev |
| Branch Preview URL: | https://feature-search-dynamic-edge.vald.pages.dev |
Matts966
force-pushed
the
feature/search/dynamic-edge-size
branch
from
2bbcfa2 to
f395008
Compare
|
/format |
|
[FORMAT] Updating license headers and formatting go codes triggered by Matts966. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/core/algorithm/ngt/ngt.go (2)
638-668:⚠️ Potential issue | 🟠 Major
edgeSizehas no zero-value fallback, unlikeepsilonandradius.Lines 651–657 apply fallbacks for both
epsilonandradiuswhen the caller passes0:if epsilon == 0 { epsilon = n.epsilon } if radius == 0 { radius = n.radius }
edgeSizereceives no equivalent treatment — a zero value is forwarded directly toC.ngt_search_index_as_float. Whether0is safe depends entirely on the C API's contract (e.g., "use index-configured default" vs. "disable dynamic edge sizing" vs. undefined behaviour). There is no storededgeSizedefault on thengtstruct and no negative-value guard either.At minimum, the semantics of
edgeSize == 0must be documented explicitly. If0is not a meaningful no-op, the pattern should mirrorepsilon/radius:💡 Proposed alignment with existing fallback pattern
+ // ces is used as the edge-size-for-creation; a dedicated search default + // would need a new field (e.g. n.searchEdgeSize) populated from options. + // For now, -1 is the NGT convention for "use index default". + if edgeSize == 0 { + edgeSize = -1 // or a stored n.searchEdgeSize default + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/algorithm/ngt/ngt.go` around lines 638 - 668, The code forwards edgeSize==0 directly to C.ngt_search_index_as_float while epsilon and radius fall back to n.epsilon/n.radius; add a consistent fallback or guard for edgeSize: either introduce a default field on the ngt struct (e.g., n.edgeSize) and set edgeSize = n.edgeSize when edgeSize == 0, or explicitly validate and reject zero (return an error) before calling ngt_search_index_as_float; update the function that calls C.ngt_search_index_as_float to apply this fallback/validation and adjust any callers/tests to use the new behavior.
660-669:⚠️ Potential issue | 🔴 CriticalResolve NGT API signature mismatch before merge.
The
ngt_search_index_as_floatcall passes 9 arguments withedgeSizeas the 8th parameter, but the NGT main branch signature only accepts 8 parameters and does not includeedgeSize. The feature branchfeature/search/dynamic-edge-size(which introduces this parameter) is inaccessible (404). This parameter-order mismatch causes silent memory corruption in CGo—the Go compiler will not detect it.Confirm that:
- The NGT feature branch
feature/search/dynamic-edge-sizeexists and is publicly accessible, or- The edgeSize parameter has been merged into NGT's main branch, or
- Update the Go call to match the current NGT C API signature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/algorithm/ngt/ngt.go` around lines 660 - 669, The call to C.ngt_search_index_as_float in function/method using n.index is passing an extra edgeSize argument (9 args) which doesn't match the mainline NGT C API (8 args) and causes silent CGo memory corruption; either confirm the remote feature branch or merged API, but most likely remove the edgeSize parameter and call ngt_search_index_as_float with the current 8-argument signature (index, pointer to vec, dimension, pointer to size, pointer to epsilon, pointer to radius, results, ne.err) and update any surrounding code/comments accordingly, and run a full build/test to ensure the Cgo signature (and any typedefs/unsafe.Pointer casts) matches the NGT C header used at link time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/core/algorithm/ngt/ngt.go`:
- Around line 638-668: The code forwards edgeSize==0 directly to
C.ngt_search_index_as_float while epsilon and radius fall back to
n.epsilon/n.radius; add a consistent fallback or guard for edgeSize: either
introduce a default field on the ngt struct (e.g., n.edgeSize) and set edgeSize
= n.edgeSize when edgeSize == 0, or explicitly validate and reject zero (return
an error) before calling ngt_search_index_as_float; update the function that
calls C.ngt_search_index_as_float to apply this fallback/validation and adjust
any callers/tests to use the new behavior.
- Around line 660-669: The call to C.ngt_search_index_as_float in
function/method using n.index is passing an extra edgeSize argument (9 args)
which doesn't match the mainline NGT C API (8 args) and causes silent CGo memory
corruption; either confirm the remote feature branch or merged API, but most
likely remove the edgeSize parameter and call ngt_search_index_as_float with the
current 8-argument signature (index, pointer to vec, dimension, pointer to size,
pointer to epsilon, pointer to radius, results, ne.err) and update any
surrounding code/comments accordingly, and run a full build/test to ensure the
Cgo signature (and any typedefs/unsafe.Pointer casts) matches the NGT C header
used at link time.
Matts966
force-pushed
the
feature/search/dynamic-edge-size
branch
from
794fb97 to
3cd380c
Compare
|
/format |
|
[FORMAT] Updating license headers and formatting go codes triggered by Matts966. |
Signed-off-by: Vdaas CI <vald@vdaas.org>
Signed-off-by: Matts966 <matts966@vdaas.org>
|
/format |
|
[FORMAT] Updating license headers and formatting go codes triggered by Matts966. |
Signed-off-by: Vdaas CI <vald@vdaas.org>
|
/format |
|
[FORMAT] Updating license headers and formatting go codes triggered by Matts966. |
Signed-off-by: Vdaas CI <vald@vdaas.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 31 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Matts966
requested review from
a team,
kmrmt and
kpango
and removed request for
a team
* Support dynamic edge size parameter for Search API * 🤖 Update license headers / Format go codes and yaml files Signed-off-by: Vdaas CI <vald@vdaas.org> * Fix files * Update internal/core/algorithm/ngt/ngt.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Matts966 <mattsmasa96604a@gmail.com> * Fix arg order * 🤖 Update license headers / Format go codes and yaml files Signed-off-by: Vdaas CI <vald@vdaas.org> * 🤖 Update license headers / Format go codes and yaml files Signed-off-by: Vdaas CI <vald@vdaas.org> * Upgrade and use default NGT * 🤖 Update license headers / Format go codes and yaml files Signed-off-by: Vdaas CI <vald@vdaas.org> --------- Signed-off-by: Vdaas CI <vald@vdaas.org> Signed-off-by: Matts966 <mattsmasa96604a@gmail.com> Signed-off-by: Matts966 <matts966@vdaas.org> Co-authored-by: Vdaas CI <vald@vdaas.org> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Support dynamic edge size parameter for Search API * 🤖 Update license headers / Format go codes and yaml files * Fix files * Update internal/core/algorithm/ngt/ngt.go * Fix arg order * 🤖 Update license headers / Format go codes and yaml files * 🤖 Update license headers / Format go codes and yaml files * Upgrade and use default NGT * 🤖 Update license headers / Format go codes and yaml files --------- Signed-off-by: Vdaas CI <vald@vdaas.org> Signed-off-by: Matts966 <mattsmasa96604a@gmail.com> Signed-off-by: Matts966 <matts966@vdaas.org> Co-authored-by: Matts966 <matts966@vdaas.org> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
[FOSSA] The scan result will be available at https://app.fossa.com/projects/custom%2B21465%2Fvald/refs/branch/feature%2Fsearch%2Fdynamic-edge-size/70de68426dc696a3ab546610d3570f7d29adbc44 |
Profile Report
|
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
vdaas/ngt'sdynamic-edge-sizebranchSummary by CodeRabbit
New Features
Documentation
Chores
Refactor