Skip to content

fix: expand Pyramid ef search for large topk#2162

Merged
wxyucs merged 1 commit into
antgroup:mainfrom
jac0626:codex/fix-pyramid-topk-ef
Jun 8, 2026
Merged

fix: expand Pyramid ef search for large topk#2162
wxyucs merged 1 commit into
antgroup:mainfrom
jac0626:codex/fix-pyramid-topk-ef

Conversation

@jac0626
Copy link
Copy Markdown
Collaborator

@jac0626 jac0626 commented Jun 8, 2026

Change Type

  • Bug fix
  • New feature
  • Improvement/Refactor
  • Documentation
  • CI/Build/Infra

Linked Issue

What Changed

  • Expand Pyramid KNN effective ef to at least topk, matching HGraph behavior when topk > ef_search.
  • Add a Pyramid regression test covering ef_search < topk and asserting KNN returns topk results when enough data exists.

Test Evidence

  • make fmt
  • make lint
  • make test
  • make cov, run tests, and collect coverage
  • Other (describe below)

Test details:

PATH=/opt/homebrew/opt/llvm@15/bin:$PATH make fmt
Using clang-format version 15 (required: 15)
Code formatting completed with /opt/homebrew/opt/llvm@15/bin/clang-format

Local tests were not run per maintainer request.

Compatibility Impact

  • API/ABI compatibility: none
  • Behavior changes: Pyramid KNN can search more candidates when topk exceeds ef_search, so it can return the requested topk results instead of being capped by ef_search.

Performance and Concurrency Impact

  • Performance impact: searches with topk > ef_search may do more work, consistent with the larger requested result count.
  • Concurrency/thread-safety impact: none

Documentation Impact

  • No docs update needed
  • Updated docs:
    • README.md
    • DEVELOPMENT.md
    • CONTRIBUTING.md
    • Other:

Risk and Rollback

  • Risk level: low
  • Rollback plan: revert this commit to restore the previous Pyramid KNN candidate cap behavior.

Checklist

  • I have linked the relevant issue (required for kind/bug and kind/feature; see "Linked Issue" above)
  • I have added/updated tests for new behavior or bug fixes
  • I have considered API compatibility impact
  • I have updated docs if behavior/workflow changed
  • My commit messages follow project conventions (Conventional Commits, optional [skip ci] prefix)

Copilot AI review requested due to automatic review settings June 8, 2026 08:09
@jac0626 jac0626 added kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) version/1.0 labels Jun 8, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR ensures Pyramid KNN search uses an effective ef that is at least topk, preventing searches from returning fewer than k results when ef_search < k.

Changes:

  • Update Pyramid KnnSearch to set internal ef to max(ef_search, k).
  • Add a functional test that asserts KnnSearch returns topk results even when ef_search is smaller than topk.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/test_pyramid.cpp Adds coverage for the ef_search < topk scenario to ensure topk results are returned.
src/algorithm/pyramid.cpp Adjusts internal search parameters so ef is at least k.

Comment thread src/algorithm/pyramid/pyramid.cpp
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Jun 8, 2026

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Require kind label

Wonderful, this rule succeeded.
  • label~=^kind/

🟢 Require version label

Wonderful, this rule succeeded.
  • label~=^version/

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request ensures that search_param.ef is at least as large as k by setting it to the maximum of parsed_param.ef_search and k, and adds a corresponding unit test. Feedback suggests clamping k to at least 0 before casting to uint64_t to prevent potential wrap-around issues if k is negative.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/algorithm/pyramid/pyramid.cpp
@jac0626 jac0626 force-pushed the codex/fix-pyramid-topk-ef branch from 7c2d9a1 to aceb8ff Compare June 8, 2026 08:19
Signed-off-by: JiangChao <jacllovey@qq.com>
Assisted-by: Codex:GPT-5
@jac0626 jac0626 force-pushed the codex/fix-pyramid-topk-ef branch from aceb8ff to 9e34107 Compare June 8, 2026 08:23
@wxyucs wxyucs self-assigned this Jun 8, 2026
Copy link
Copy Markdown
Collaborator

@wxyucs wxyucs left a comment

Choose a reason for hiding this comment

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

lgtm

@wxyucs wxyucs merged commit 97489d4 into antgroup:main Jun 8, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) module/testing size/M version/1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent handling of topk > ef_search between HGraph and Pyramid

3 participants