Skip to content

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.

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

Merged
merged 1 commit into from
Aug 6, 2025
Merged

fix/fixing filtering by index for n by index #332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions helix-db/src/helix_engine/graph_core/ops/source/n_from_index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use crate::{
},
protocol::value::Value,
};
use heed3::{RoTxn, byteorder::BE};
use helix_macros::debug_trace;
use heed3::{byteorder::BE, RoTxn};
use serde::Serialize;
use std::sync::Arc;

Expand All @@ -16,18 +16,25 @@ pub struct NFromIndex<'a> {
heed3::RoPrefix<'a, heed3::types::Bytes, heed3::types::LazyDecode<heed3::types::U128<BE>>>,
txn: &'a RoTxn<'a>,
storage: Arc<HelixGraphStorage>,
label: &'a str,
}

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() {
Copy link
Preview

Copilot AI Aug 6, 2025

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.

Suggested change
for value in self.iter.by_ref() {
while let Some(value) = self.iter.next() {

Copilot uses AI. Check for mistakes.

let (_, value) = value.unwrap();
match value.decode() {
Ok(value) => match self.storage.get_node(self.txn, &value) {
Ok(node) => return Some(Ok(TraversalVal::Node(node))),
Ok(node) => {
if node.label == self.label {
return Some(Ok(TraversalVal::Node(node)));
} else {
continue;
Comment on lines +34 to +35
Copy link
Preview

Copilot AI Aug 6, 2025

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.

Suggested change
} else {
continue;

Copilot uses AI. Check for mistakes.

}
}
Err(e) => {
println!("{} Error getting node: {:?}", line!(), e);
return Some(Err(GraphError::ConversionError(e.to_string())));
Expand Down Expand Up @@ -55,7 +62,7 @@ pub trait NFromIndexAdapter<'a, K: Into<Value> + Serialize>:
///
/// Note that both the `index` and `key` must be provided.
/// The index must be a valid and existing secondary index and the key should match the type of the index.
fn n_from_index(self, index: &'a str, key: &'a K) -> Self::OutputIter
fn n_from_index(self, label: &'a str, index: &'a str, key: &'a K) -> Self::OutputIter
where
K: Into<Value> + Serialize + Clone;
}
Expand All @@ -66,7 +73,7 @@ impl<'a, I: Iterator<Item = Result<TraversalVal, GraphError>>, K: Into<Value> +
type OutputIter = RoTraversalIterator<'a, NFromIndex<'a>>;

#[inline]
fn n_from_index(self, index: &'a str, key: &'a K) -> Self::OutputIter
fn n_from_index(self, label: &'a str, index: &'a str, key: &'a K) -> Self::OutputIter
where
K: Into<Value> + Serialize + Clone,
{
Expand All @@ -88,6 +95,7 @@ impl<'a, I: Iterator<Item = Result<TraversalVal, GraphError>>, K: Into<Value> +
iter: res,
txn: self.txn,
storage: Arc::clone(&self.storage),
label,
};

RoTraversalIterator {
Expand Down
4 changes: 2 additions & 2 deletions helix-db/src/helix_engine/graph_core/traversal_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2708,7 +2708,7 @@ fn test_update_of_secondary_indices() {
let txn = storage.graph_env.read_txn().unwrap();

let node = G::new(Arc::clone(&storage), &txn)
.n_from_index("name", &"Jane".to_string())
.n_from_index("person", "name", &"Jane".to_string())
.collect_to::<Vec<_>>();
assert_eq!(node.len(), 1);
assert_eq!(node[0].id(), node.id());
Expand All @@ -2722,7 +2722,7 @@ fn test_update_of_secondary_indices() {
}

let node = G::new(Arc::clone(&storage), &txn)
.n_from_index("name", &"John".to_string())
.n_from_index("person", "name", &"John".to_string())
.collect_to::<Vec<_>>();
assert_eq!(node.len(), 0);

Expand Down
1 change: 1 addition & 0 deletions helix-db/src/helixc/analyzer/methods/traversal_validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ pub(crate) fn validate_traversal<'a>(
};
gen_traversal.source_step =
Separator::Period(SourceStep::NFromIndex(NFromIndex {
label: GenRef::Literal(node_type.clone()),
index: GenRef::Literal(match *index {
IdType::Identifier { value, loc: _ } => value,
// would be caught by the parser
Expand Down
3 changes: 2 additions & 1 deletion helix-db/src/helixc/generator/source_steps.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,11 +196,12 @@ impl Display for SearchVector {
pub struct NFromIndex {
pub index: GenRef<String>,
pub key: GeneratedValue,
pub label: GenRef<String>,
}

impl Display for NFromIndex {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "n_from_index({}, {})", self.index, self.key)
write!(f, "n_from_index({}, {}, {})", self.label, self.index, self.key)
}
}

Loading