Skip to content

feat: add lookup_index_by_key to Rust Vector for index-based search#8959

Merged
jtdavis777 merged 4 commits into
google:masterfrom
statxc:rust/add-lookup-index-by-key
Mar 11, 2026
Merged

feat: add lookup_index_by_key to Rust Vector for index-based search#8959
jtdavis777 merged 4 commits into
google:masterfrom
statxc:rust/add-lookup-index-by-key

Conversation

@statxc
Copy link
Copy Markdown
Contributor

@statxc statxc commented Mar 6, 2026

Closes: #8667

Problem

lookup_by_key performs binary search on sorted vectors but can only return the matched object. There is no way to get the element's index, which is needed when using vector positions as references (e.g., graph adjacency lists, ECS entity lookups, index-based cross-referencing between vectors).

Fix

Add lookup_index_by_key to Vector - identical binary search logic as lookup_by_key, but returns Option<usize> (the index) instead of Option<T::Inner> (the object).

Changes

  • rust/flatbuffers/src/vector.rs - add lookup_index_by_key method on Vector<'a, T>
  • tests/rust_usage_test/tests/integration_test.rs - add 5 test cases for the new method

Test plan

# Run new tests
cd tests/rust_usage_test && cargo test lookup_index_by_key

# Run full test suite
cd tests/rust_usage_test && cargo test

@statxc statxc requested a review from dbaileychess as a code owner March 6, 2026 21:25
@github-actions github-actions Bot added the rust label Mar 6, 2026
@statxc
Copy link
Copy Markdown
Contributor Author

statxc commented Mar 6, 2026

@jtdavis777 I'd appreciate you also review this PR.

Comment thread rust/flatbuffers/src/vector.rs
@statxc
Copy link
Copy Markdown
Contributor Author

statxc commented Mar 7, 2026

Thanks for your feedback @jtdavis777 . I removed duplicated codes. Please review again

@statxc
Copy link
Copy Markdown
Contributor Author

statxc commented Mar 7, 2026

@jtdavis777 Any update for me, please

@statxc
Copy link
Copy Markdown
Contributor Author

statxc commented Mar 10, 2026

@jtdavis777 Please let me know any feedbacks. what else should I update?

jtdavis777
jtdavis777 previously approved these changes Mar 11, 2026
@jtdavis777
Copy link
Copy Markdown
Collaborator

@statxc looks like there is a merge conflict in the tests. can you merge or rebase on master for me?

@statxc
Copy link
Copy Markdown
Contributor Author

statxc commented Mar 11, 2026

@jtdavis777 Fixed conflicts. Could you run CI testing?

@jtdavis777 jtdavis777 enabled auto-merge (squash) March 11, 2026 02:11
@jtdavis777 jtdavis777 merged commit a7fed2c into google:master Mar 11, 2026
48 of 50 checks passed
@statxc statxc deleted the rust/add-lookup-index-by-key branch March 11, 2026 02:15
@statxc statxc restored the rust/add-lookup-index-by-key branch March 11, 2026 02:19
@statxc
Copy link
Copy Markdown
Contributor Author

statxc commented Mar 11, 2026

@jtdavis777 I saw testing error. I found problem - I missed closing bracket } in integration_test.rs file while fixing conflicts.
how can I update this?

@statxc
Copy link
Copy Markdown
Contributor Author

statxc commented Mar 11, 2026

Should I send another pr for this? please let me know your best option

@jtdavis777
Copy link
Copy Markdown
Collaborator

interesting this wasn't caught by the tests themselves.. did they not run properly? can you inspect the testing logs?

Just submit a new PR, no need for a new issue just reference your comment.

@statxc
Copy link
Copy Markdown
Contributor Author

statxc commented Mar 11, 2026

@jtdavis777
https://github.com/google/flatbuffers/actions/runs/22933105103

please check this. it is testing error log.

I will submit new pr for this.

@jtdavis777
Copy link
Copy Markdown
Collaborator

ahh sure enough. Do we not have those tests marked as required?? Lovely. Thank you for being thorough, looking forward to the fix PR. I only have a little bit of time each day to review but we can get this in as soon as possible.

@statxc statxc deleted the rust/add-lookup-index-by-key branch March 11, 2026 02:32
@statxc
Copy link
Copy Markdown
Contributor Author

statxc commented Mar 11, 2026

@jtdavis777 Fixed error. Please review this PR: #8969

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.

[Rust] Add support for binary search returning the index

2 participants