-
-
Notifications
You must be signed in to change notification settings - Fork 115
fix/fixing filtering by index for n by index #332
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the filtering functionality for the n_from_index
operation by adding label-based filtering to ensure only nodes with the correct label are returned from index queries.
- Added a
label
parameter to theNFromIndex
struct and related operations - Modified the iterator implementation to filter nodes by label before returning them
- Updated the Display format and test cases to use the new three-parameter signature
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
helix-db/src/helixc/generator/source_steps.rs |
Added label field to NFromIndex struct and updated Display format |
helix-db/src/helixc/analyzer/methods/traversal_validation.rs |
Added label parameter when constructing NFromIndex instances |
helix-db/src/helix_engine/graph_core/traversal_tests.rs |
Updated test calls to use new three-parameter n_from_index signature |
helix-db/src/helix_engine/graph_core/ops/source/n_from_index.rs |
Implemented label filtering logic and updated API signature |
} | ||
|
||
impl<'a> Iterator for NFromIndex<'a> { | ||
type Item = Result<TraversalVal, GraphError>; | ||
|
||
#[debug_trace("N_FROM_INDEX")] | ||
fn next(&mut self) -> Option<Self::Item> { | ||
if let Some(value) = self.iter.by_ref().next() { | ||
for value in self.iter.by_ref() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from if let Some(value)
to for value in
alters the iterator behavior. The original code processed one item per next()
call, while this new code will potentially consume multiple items in a single next()
call, which could impact performance and memory usage when dealing with large result sets.
for value in self.iter.by_ref() { | |
while let Some(value) = self.iter.next() { |
Copilot uses AI. Check for mistakes.
} else { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The continue
statement is unnecessary since it's at the end of the loop iteration. The else branch can be removed entirely as the loop will naturally continue to the next iteration.
} else { | |
continue; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected
Description
fixing filtering by index for n by index
Related Issues
Closes #
Checklist when merging to main
rustfmt
helix-cli/Cargo.toml
andhelixdb/Cargo.toml
Additional Notes