Skip to content

[ENH] Split knn orchestrators for reuse #5347

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Sep 2, 2025
Merged

[ENH] Split knn orchestrators for reuse #5347

merged 13 commits into from
Sep 2, 2025
+825 −311

Conversation

Sicheng-Pan
Copy link
Contributor

@Sicheng-Pan Sicheng-Pan commented Aug 22, 2025 •
edited
Loading

Description of changes

Summarize the changes made by this PR.

  • Improvements & Bug fixes
    • N/A
  • New functionality
    • Separate out the projection operator from KnnOrchestrator and SpannKnnOrchestrator, so that we can reuse these orchestrators for search endpoint

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Migration plan

Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?

Observability plan

What is the plan to instrument and monitor this change?

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?

@Sicheng-Pan Sicheng-Pan mentioned this pull request Aug 22, 2025
Merged
1 task
@github-actions GitHub Actions
Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@Sicheng-Pan Sicheng-Pan mentioned this pull request Aug 22, 2025
Merged
1 task
@Sicheng-Pan Graphite App
Copy link
Contributor Author

Sicheng-Pan commented Aug 22, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Sicheng-Pan Sicheng-Pan mentioned this pull request Aug 22, 2025
Merged
1 task
@Sicheng-Pan Sicheng-Pan marked this pull request as ready for review August 22, 2025 22:24
@Sicheng-Pan Sicheng-Pan marked this pull request as draft August 22, 2025 22:24
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch from c8592d1 to 91db19f Compare August 22, 2025 22:30
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch 2 times, most recently from 6921a82 to 682b7cc Compare August 22, 2025 23:42
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch 2 times, most recently from 4b1be8f to 613d378 Compare August 23, 2025 04:59
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch from 682b7cc to 373ebd8 Compare August 23, 2025 04:59
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch from 613d378 to 5a95d37 Compare August 23, 2025 05:23
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch 2 times, most recently from 4afa332 to daecf23 Compare August 25, 2025 17:26
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch from 5a95d37 to 2cdad7a Compare August 25, 2025 17:26
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch from 4944575 to ffb6f57 Compare August 25, 2025 17:44
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch from 2cdad7a to b6d6bdd Compare August 25, 2025 17:44
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch from ffb6f57 to 239e413 Compare August 25, 2025 19:03
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch from b6d6bdd to 2fcfdb1 Compare August 25, 2025 19:03
@Sicheng-Pan Sicheng-Pan changed the title [ENH] Implement retrieve orchestrartor [ENH] Implement search orchestrartor Aug 25, 2025
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch 2 times, most recently from 45afad9 to 4fe7e46 Compare August 25, 2025 21:12
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch from 3cca0bf to dfc4886 Compare August 25, 2025 21:12
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch from 4fe7e46 to f07fb90 Compare August 25, 2025 21:27
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch from dfc4886 to e962bf5 Compare August 25, 2025 21:27
@Sicheng-Pan Sicheng-Pan changed the title [ENH] Implement search orchestrartor [ENH] Implement search orchestrartors Aug 25, 2025
@Sicheng-Pan Sicheng-Pan changed the title [ENH] Implement search orchestrartors [ENH] Split knn orchestrators for reuse Aug 26, 2025
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch from c9ea839 to fc2d707 Compare August 26, 2025 00:21
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch from 007ddcc to 91132e9 Compare August 29, 2025 22:31
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch from e431493 to 271d53b Compare September 2, 2025 19:29
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch from dd01607 to c0ee973 Compare September 2, 2025 19:29
@Sicheng-Pan Sicheng-Pan changed the base branch from 08-20-_enh_implement_new_retrieve_endpoint to graphite-base/5347 September 2, 2025 19:47
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch from 271d53b to 49963c8 Compare September 2, 2025 19:47
@Sicheng-Pan Sicheng-Pan changed the base branch from graphite-base/5347 to 08-20-_enh_implement_new_retrieve_endpoint September 2, 2025 19:47
@Sicheng-Pan Sicheng-Pan force-pushed the 08-20-_enh_implement_new_retrieve_endpoint branch from ed688f0 to b340fe1 Compare September 2, 2025 19:59
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch from 49963c8 to 6b2004b Compare September 2, 2025 19:59
@Sicheng-Pan Sicheng-Pan changed the base branch from 08-20-_enh_implement_new_retrieve_endpoint to graphite-base/5347 September 2, 2025 20:03
@Sicheng-Pan
[ENH] Implement retrieve orchestrartor
241ec11
@Sicheng-Pan
In progress
2552867
@Sicheng-Pan
Restart from scratch
4c69fd1
@Sicheng-Pan
Implement sparse knn orchestrator
1cd68bf
@Sicheng-Pan
Pull collection information out of output
4495bcf
@Sicheng-Pan
Projection orchestrator
bd56a71
@Sicheng-Pan
Remove sparse KNN components for later PR
b8b29e4
@Sicheng-Pan
Fix merge
1286908
@Sicheng-Pan
Rename merge argument
0b00b2e
@Sicheng-Pan
Rename Limit fields
8083d42
@Sicheng-Pan
Fix cargo
9134e8d
@Sicheng-Pan
test: add comprehensive unit tests for Merge operator
3268007 
- Test basic k-way merge with Reverse integers to verify correct top-k selection
- Test deduplication behavior across and within batches
- Test edge cases: empty vectors, single vector, mixed empty/non-empty
- Test k boundary conditions (k=0, k=1, k > total elements)
- Test with different orderable types (strings, custom structs)
- Test order preservation in output
- Test all-same-values edge case for deduplication
- Fix Limit test field names after rename (skip→offset, fetch→limit)

The tests ensure the generic Merge::merge algorithm correctly handles:
- Max heap based k-way merge
- Proper ordering based on type's Ord implementation
- Deduplication of equal elements
- Edge cases and boundary conditions
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch from 6b2004b to f2ac557 Compare September 2, 2025 20:04
@graphite-app graphite-app bot changed the base branch from graphite-base/5347 to main September 2, 2025 20:04
@Sicheng-Pan
test: diversify Merge operator tests with more integer types
ee62311 
- Replace most Reverse<u32> tests with direct integer types (u32, i32, i64, u128)
- Test with regular descending order integers instead of always using Reverse wrapper
- Add tests with signed integers including negative values
- Remove float tests since they don't implement Ord trait
- Keep mix of Reverse tests and direct integer tests for better coverage

The tests now better demonstrate the generic nature of the Merge algorithm
with various integer types in their natural descending order.
@Sicheng-Pan Sicheng-Pan force-pushed the 08-22-_enh_implement_retrieve_orchestrartor branch from f2ac557 to ee62311 Compare September 2, 2025 20:04
@Sicheng-Pan Graphite App
Copy link
Contributor Author

Sicheng-Pan commented Sep 2, 2025 •
edited
Loading

Merge activity

  • Sep 2, 8:37 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Sep 2, 8:37 PM UTC: @Sicheng-Pan merged this pull request with Graphite.

@Sicheng-Pan Sicheng-Pan merged commit 3ef88f8 into main Sep 2, 2025
60 checks passed
@Sicheng-Pan Sicheng-Pan deleted the 08-22-_enh_implement_retrieve_orchestrartor branch September 2, 2025 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HammadB HammadB HammadB left review comments

@sanketkedia sanketkedia sanketkedia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Sicheng-Pan @HammadB @sanketkedia