Skip to content

Commit 2338520

Browse files
xav-dbxav-db
authored
fix (hql): fixing hql with inner traversals (#718)
<!-- greptile_comment --> <h2>Greptile Overview</h2> <h3>Greptile Summary</h3> Optimized HQL compiler to handle inner traversals in WHERE clause comparisons. Previously, comparing properties from different variables (e.g., `_::{login}::EQ(toUser::{login})`) would generate inefficient code with unnecessary `G::from_iter` calls. The fix introduces `PropertyEq` and `PropertyNeq` operators that detect simple property access patterns and generate direct `get_property()` calls instead. **Key Changes:** - Added `is_simple_property_traversal()` helper that identifies property-only traversals (no graph navigation) - Created `PropertyEq` and `PropertyNeq` boolean operators for optimized property comparisons - Updated Equal/NotEqual handling in `traversal_validation.rs` to use optimized path for simple cases - Added comprehensive test coverage with GitHub graph and document embedding schemas **Impact:** - Enables queries like `CheckFollowsEdge` that filter edges by comparing properties across variables - Generates more efficient Rust code for common property comparison patterns - Maintains backward compatibility by falling back to full traversal parsing for complex cases <details><summary><h3>Important Files Changed</h3></summary> File Analysis | Filename | Score | Overview | |----------|-------|----------| | helix-db/src/helixc/analyzer/methods/traversal_validation.rs | 5/5 | Added `is_simple_property_traversal` helper function and optimized Equal/NotEqual operators to use PropertyEq/PropertyNeq for simple property access, avoiding unnecessary `G::from_iter` calls | | helix-db/src/helixc/generator/bool_ops.rs | 5/5 | Added PropertyEq and PropertyNeq boolean operators that generate optimized property comparison code using `get_property` instead of creating full traversals | | helix-db/src/helixc/generator/traversal_steps.rs | 5/5 | Updated WhereRef Display implementation to handle new PropertyEq/PropertyNeq boolean operators with unreachable guard for reserved properties | | hql-tests/tests/user_test_2/queries.hx | 5/5 | New GitHub-style graph queries including `CheckFollowsEdge` query that uses inner traversal comparison pattern `_::{login}::EQ(toUser::{login})` | </details> </details> <details><summary><h3>Sequence Diagram</h3></summary> ```mermaid sequenceDiagram participant User as HQL Query participant Analyzer as traversal_validation.rs participant BoolOps as bool_ops.rs participant Generator as traversal_steps.rs participant Output as Generated Rust Code User->>Analyzer: WHERE(_::{login}::EQ(toUser::{login})) Analyzer->>Analyzer: Validate BooleanOpType::Equal Analyzer->>Analyzer: Check if RHS is ExpressionType::Traversal Analyzer->>Analyzer: Call is_simple_property_traversal() alt Simple property traversal Analyzer->>Analyzer: Detected: toUser::{login} Analyzer->>BoolOps: Create PropertyEq(var: "toUser", property: "login") BoolOps->>Generator: Pass PropertyEq to WhereRef Generator->>Output: Generate: toUser.get_property("login").map_or(false, |w| w == v) else Complex traversal Analyzer->>Analyzer: Parse full traversal with validate_traversal() Analyzer->>BoolOps: Create Eq with GeneratedValue::Traversal BoolOps->>Generator: Pass Eq with traversal Generator->>Output: Generate: G::from_iter(...) comparison end ``` </details> <!-- greptile_other_comments_section --> <!-- /greptile_comment -->
2 parents e274a5d + 9dedc63 commit 2338520

File tree

10 files changed

+501
-41
lines changed

10 files changed

+501
-41
lines changed

‎helix-db/src/helixc/analyzer/methods/traversal_validation.rs‎

Lines changed: 96 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use crate::helixc::analyzer::error_codes::*;
22
use crate::helixc::analyzer::utils::{
33
DEFAULT_VAR_NAME, VariableInfo, check_identifier_is_fieldtype,
44
};
5-
use crate::helixc::generator::bool_ops::{Contains, IsIn};
5+
use crate::helixc::generator::bool_ops::{Contains, IsIn, PropertyEq, PropertyNeq};
66
use crate::helixc::generator::source_steps::{SearchVector, VFromID, VFromType};
77
use crate::helixc::generator::traversal_steps::{AggregateBy, GroupBy};
88
use crate::helixc::generator::utils::{EmbedData, VecData};
@@ -94,6 +94,45 @@ fn get_reserved_property_type(prop_name: &str, item_type: &Type) -> Option<Field
9494
}
9595
}
9696

97+
/// Checks if a traversal is a "simple" property access (no graph navigation steps)
98+
/// and returns the variable name and property name if so.
99+
///
100+
/// A simple traversal is one that only accesses properties on an already-bound variable,
101+
/// without any graph navigation (Out, In, etc.). For example: `toUser::{login}`
102+
///
103+
/// Returns: Some((variable_name, property_name)) if simple, None otherwise
104+
fn is_simple_property_traversal(tr: &Traversal) -> Option<(String, String)> {
105+
// Check if the start is an identifier (not a type-based query)
106+
let var_name = match &tr.start {
107+
StartNode::Identifier(id) => id.clone(),
108+
_ => return None,
109+
};
110+
111+
// Check if there's exactly one step and it's an Object (property access)
112+
if tr.steps.len() != 1 {
113+
return None;
114+
}
115+
116+
// Check if the single step is an Object step (property access like {login})
117+
match &tr.steps[0].step {
118+
StepType::Object(obj) => {
119+
// Check if it's a simple property fetch (single field, no spread)
120+
if obj.fields.len() == 1 && !obj.should_spread {
121+
let field = &obj.fields[0];
122+
// Check if it's a simple field selection (Empty or Identifier, not a complex expression)
123+
match &field.value.value {
124+
FieldValueType::Empty | FieldValueType::Identifier(_) => {
125+
return Some((var_name, field.key.clone()));
126+
}
127+
_ => return None,
128+
}
129+
}
130+
None
131+
}
132+
_ => None,
133+
}
134+
}
135+
97136
/// Validates the traversal and returns the end type of the traversal
98137
///
99138
/// This method also builds the generated traversal (`gen_traversal`) as it analyzes the traversal
@@ -1170,7 +1209,32 @@ pub(crate) fn validate_traversal<'a>(
11701209
})
11711210
}
11721211
BooleanOpType::Equal(expr) => {
1173-
let v = match &expr.expr {
1212+
// Check if the right-hand side is a simple property traversal
1213+
if let ExpressionType::Traversal(traversal) = &expr.expr {
1214+
if let Some((var, property)) = is_simple_property_traversal(traversal) {
1215+
// Use PropertyEq for simple traversals to avoid unnecessary G::from_iter
1216+
BoolOp::PropertyEq(PropertyEq { var, property })
1217+
} else {
1218+
// Complex traversal - parse normally
1219+
let mut gen_traversal = GeneratedTraversal::default();
1220+
validate_traversal(
1221+
ctx,
1222+
traversal,
1223+
scope,
1224+
original_query,
1225+
parent_ty.clone(),
1226+
&mut gen_traversal,
1227+
gen_query,
1228+
);
1229+
gen_traversal.should_collect = ShouldCollect::ToValue;
1230+
let v = GeneratedValue::Traversal(Box::new(gen_traversal));
1231+
BoolOp::Eq(Eq {
1232+
left: GeneratedValue::Primitive(GenRef::Std("*v".to_string())),
1233+
right: v,
1234+
})
1235+
}
1236+
} else {
1237+
let v = match &expr.expr {
11741238
ExpressionType::BooleanLiteral(b) => {
11751239
GeneratedValue::Primitive(GenRef::Std(b.to_string()))
11761240
}
@@ -1192,8 +1256,24 @@ pub(crate) fn validate_traversal<'a>(
11921256
);
11931257
gen_identifier_or_param(original_query, i.as_str(), false, true)
11941258
}
1195-
ExpressionType::Traversal(traversal) => {
1196-
// parse traversal
1259+
_ => {
1260+
unreachable!("Cannot reach here");
1261+
}
1262+
};
1263+
BoolOp::Eq(Eq {
1264+
left: GeneratedValue::Primitive(GenRef::Std("*v".to_string())),
1265+
right: v,
1266+
})
1267+
}
1268+
}
1269+
BooleanOpType::NotEqual(expr) => {
1270+
// Check if the right-hand side is a simple property traversal
1271+
if let ExpressionType::Traversal(traversal) = &expr.expr {
1272+
if let Some((var, property)) = is_simple_property_traversal(traversal) {
1273+
// Use PropertyNeq for simple traversals to avoid unnecessary G::from_iter
1274+
BoolOp::PropertyNeq(PropertyNeq { var, property })
1275+
} else {
1276+
// Complex traversal - parse normally
11971277
let mut gen_traversal = GeneratedTraversal::default();
11981278
validate_traversal(
11991279
ctx,
@@ -1205,19 +1285,14 @@ pub(crate) fn validate_traversal<'a>(
12051285
gen_query,
12061286
);
12071287
gen_traversal.should_collect = ShouldCollect::ToValue;
1208-
GeneratedValue::Traversal(Box::new(gen_traversal))
1209-
}
1210-
_ => {
1211-
unreachable!("Cannot reach here");
1288+
let v = GeneratedValue::Traversal(Box::new(gen_traversal));
1289+
BoolOp::Neq(Neq {
1290+
left: GeneratedValue::Primitive(GenRef::Std("*v".to_string())),
1291+
right: v,
1292+
})
12121293
}
1213-
};
1214-
BoolOp::Eq(Eq {
1215-
left: GeneratedValue::Primitive(GenRef::Std("*v".to_string())),
1216-
right: v,
1217-
})
1218-
}
1219-
BooleanOpType::NotEqual(expr) => {
1220-
let v = match &expr.expr {
1294+
} else {
1295+
let v = match &expr.expr {
12211296
ExpressionType::BooleanLiteral(b) => {
12221297
GeneratedValue::Primitive(GenRef::Std(b.to_string()))
12231298
}
@@ -1239,27 +1314,13 @@ pub(crate) fn validate_traversal<'a>(
12391314
);
12401315
gen_identifier_or_param(original_query, i.as_str(), false, true)
12411316
}
1242-
ExpressionType::Traversal(traversal) => {
1243-
// parse traversal
1244-
let mut gen_traversal = GeneratedTraversal::default();
1245-
validate_traversal(
1246-
ctx,
1247-
traversal,
1248-
scope,
1249-
original_query,
1250-
parent_ty.clone(),
1251-
&mut gen_traversal,
1252-
gen_query,
1253-
);
1254-
gen_traversal.should_collect = ShouldCollect::ToValue;
1255-
GeneratedValue::Traversal(Box::new(gen_traversal))
1256-
}
12571317
_ => unreachable!("Cannot reach here"),
12581318
};
1259-
BoolOp::Neq(Neq {
1260-
left: GeneratedValue::Primitive(GenRef::Std("*v".to_string())),
1261-
right: v,
1262-
})
1319+
BoolOp::Neq(Neq {
1320+
left: GeneratedValue::Primitive(GenRef::Std("*v".to_string())),
1321+
right: v,
1322+
})
1323+
}
12631324
}
12641325
BooleanOpType::Contains(expr) => {
12651326
let v = match &expr.expr {

‎helix-db/src/helixc/generator/bool_ops.rs‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ pub enum BoolOp {
1818
Neq(Neq),
1919
Contains(Contains),
2020
IsIn(IsIn),
21+
PropertyEq(PropertyEq),
22+
PropertyNeq(PropertyNeq),
2123
}
2224
impl Display for BoolOp {
2325
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
@@ -30,6 +32,8 @@ impl Display for BoolOp {
3032
BoolOp::Neq(neq) => format!("{neq}"),
3133
BoolOp::Contains(contains) => format!("v{contains}"),
3234
BoolOp::IsIn(is_in) => format!("v{is_in}"),
35+
BoolOp::PropertyEq(prop_eq) => format!("{prop_eq}"),
36+
BoolOp::PropertyNeq(prop_neq) => format!("{prop_neq}"),
3337
};
3438
write!(f, "map_value_or(false, |v| {s})?")
3539
}
@@ -100,6 +104,28 @@ impl Display for Neq {
100104
}
101105
}
102106

107+
#[derive(Clone, Debug)]
108+
pub struct PropertyEq {
109+
pub var: String,
110+
pub property: String,
111+
}
112+
impl Display for PropertyEq {
113+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
114+
write!(f, "{}.get_property(\"{}\").map_or(false, |w| w == v)", self.var, self.property)
115+
}
116+
}
117+
118+
#[derive(Clone, Debug)]
119+
pub struct PropertyNeq {
120+
pub var: String,
121+
pub property: String,
122+
}
123+
impl Display for PropertyNeq {
124+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
125+
write!(f, "{}.get_property(\"{}\").map_or(false, |w| w != v)", self.var, self.property)
126+
}
127+
}
128+
103129
#[derive(Clone, Debug)]
104130
pub struct Contains {
105131
pub value: GeneratedValue,
@@ -241,6 +267,8 @@ impl Display for BoExp {
241267
BoolOp::Neq(neq) => format!("{neq}"),
242268
BoolOp::Contains(contains) => format!("v{contains}"),
243269
BoolOp::IsIn(is_in) => format!("v{is_in}"),
270+
BoolOp::PropertyEq(prop_eq) => format!("{prop_eq}"),
271+
BoolOp::PropertyNeq(prop_neq) => format!("{prop_neq}"),
244272
};
245273
return write!(
246274
f,

‎helix-db/src/helixc/generator/traversal_steps.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,9 @@ impl Display for WhereRef {
476476
BoolOp::Neq(neq) => format!("{} != {}", value_expr, neq.right),
477477
BoolOp::Contains(contains) => format!("{}{}", value_expr, contains),
478478
BoolOp::IsIn(is_in) => format!("{}{}", value_expr, is_in),
479+
BoolOp::PropertyEq(_) | BoolOp::PropertyNeq(_) => {
480+
unreachable!("PropertyEq/PropertyNeq should not be used with reserved properties")
481+
}
479482
};
480483
return write!(
481484
f,
@@ -501,6 +504,8 @@ impl Display for WhereRef {
501504
BoolOp::Neq(neq) => format!("{neq}"),
502505
BoolOp::Contains(contains) => format!("v{contains}"),
503506
BoolOp::IsIn(is_in) => format!("v{is_in}"),
507+
BoolOp::PropertyEq(prop_eq) => format!("{prop_eq}"),
508+
BoolOp::PropertyNeq(prop_neq) => format!("{prop_neq}"),
504509
};
505510
return write!(
506511
f,

‎hql-tests/test.sh‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,16 +11,12 @@ fi
1111
file_name=$1
1212

1313

14-
helix compile --path "/Users/xav/GitHub/helix-db/hql-tests/tests/$file_name" --output "/Users/xav/GitHub/helix-db/helix-container/src"
15-
output=$(cargo check --manifest-path "/Users/xav/GitHub/helix-db/helix-container/Cargo.toml")
14+
helix compile --path "/Users/xav/GitHub/helix-db-core/hql-tests/tests/$file_name" --output "/Users/xav/GitHub/helix-db-core/helix-container/src"
15+
output=$(cargo check --manifest-path "/Users/xav/GitHub/helix-db-core/helix-container/Cargo.toml")
1616
if [ $? -ne 0 ]; then
1717
echo "Error: Cargo check failed"
1818
echo "Cargo check output: $output"
1919
exit 1
2020
fi
2121

2222
echo "Cargo check passed"
23-
24-
25-
26-

‎hql-tests/tests/user_test_2/helix.toml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
[project]
2+
name = "where_filter"
3+
queries = "."
4+
5+
[local.dev]
6+
port = 6969
7+
build_mode = "debug"
8+
9+
[cloud]

0 commit comments

Comments
 (0)