Skip to content

Commit 6a748ae

Browse files
xav-dbxav-db
authored
fix(hql+core): fixing nested ID, math ops (#844)
<!-- greptile_comment --> <h2>Greptile Overview</h2> <h3>Greptile Summary</h3> This PR implements comprehensive math operations support in HQL and fixes nested ID field access. **Key Changes:** - Implemented `Value` type arithmetic operators (`Add`, `Sub`, `Mul`, `Div`, `Rem`) with automatic type coercion between integer and float types - Added special math methods to `Value`: `pow()`, `abs()`, `sqrt()`, `min()`, `max()` - Created new `computed_expr.rs` module for generating Rust code from HQL math function calls (ADD, SUB, MUL, DIV, POW, MOD, ABS, SQRT, MIN, MAX) - Fixed nested ID access by including implicit fields when `has_spread` is true in query validation - Extended object validation to detect and store math function calls as computed expressions - Integrated computed expression generation into query code generation pipeline - Added comprehensive test suite covering all math operations with various scenarios **Previous Issues Addressed:** All previously reported missing method implementations have been resolved: - `pow(&Value)`, `abs()`, `sqrt()`, `min(&Value)`, `max(&Value)` methods implemented - `Rem` trait implemented for modulo operations - `to_f64()` helper method added (as private) for type conversions **Issues Found:** - Minor syntax issue in `min()` and `max()` code generation (missing `&` before reference parameter) <details><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | helix-db/src/protocol/value.rs | Added comprehensive math operator implementations (Add, Sub, Mul, Div, Rem) with type coercion and special methods (pow, abs, sqrt, min, max) to support computed expressions | | helix-db/src/helixc/generator/computed_expr.rs | New module for generating Rust code from math function calls in HQL queries, handles ADD, SUB, MUL, DIV, POW, MOD, ABS, SQRT, MIN, MAX | | helix-db/src/helixc/analyzer/methods/query_validation.rs | Fixed nested ID access by including implicit fields when has_spread is true, added support for computed expression fields in return types | | helix-db/src/helixc/generator/queries.rs | Integrated computed expression generation into query code generation, handling ComputedExpression field sources across single/collection/aggregate returns | | helix-db/src/helixc/analyzer/methods/object_validation.rs | Extended property access validation to detect and store MathFunctionCall expressions as computed expressions | </details> </details> <details><summary><h3>Sequence Diagram</h3></summary> ```mermaid sequenceDiagram participant HQL as HQL Query participant Parser as Parser participant ObjVal as Object Validator participant QueryVal as Query Validator participant CodeGen as Code Generator participant ComputedExpr as Computed Expr Generator participant Value as Value Type HQL->>Parser: Parse query with math expression<br/>(e.g., ADD(_::Out<HasCluster>::COUNT, 10)) Parser->>ObjVal: validate_property_access() ObjVal->>ObjVal: Detect MathFunctionCall expression ObjVal->>ObjVal: Store as ComputedExpressionInfo<br/>in traversal.computed_expressions ObjVal->>QueryVal: Continue validation QueryVal->>QueryVal: build_return_fields() QueryVal->>QueryVal: Add computed expression fields<br/>with ReturnFieldSource::ComputedExpression QueryVal->>QueryVal: Fix nested ID access:<br/>Include implicit fields when has_spread QueryVal->>CodeGen: Generate query code CodeGen->>CodeGen: Process ReturnFieldInfo CodeGen->>ComputedExpr: generate_computed_expression(expression, item_var) ComputedExpr->>ComputedExpr: Match MathFunction type<br/>(ADD, SUB, MUL, DIV, POW, etc.) ComputedExpr->>ComputedExpr: Recursively generate args ComputedExpr-->>CodeGen: Return Rust code<br/>(e.g., "(count1) + (10)") CodeGen->>CodeGen: Emit struct field initialization<br/>with computed expression Note over Value: Runtime execution Value->>Value: Execute operator (Add, Sub, Mul, Div, Rem) Value->>Value: Perform type coercion<br/>(int->float, cross-type ops) Value->>Value: Execute special methods<br/>(pow, abs, sqrt, min, max) Value-->>HQL: Return computed Value result ``` </details> <!-- greptile_other_comments_section --> <!-- /greptile_comment -->
2 parents abf1d1d + bd27f2b commit 6a748ae

File tree

21 files changed

+3390
-233
lines changed

21 files changed

+3390
-233
lines changed

‎helix-db/src/grammar.pest‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ embed_method = { "Embed" ~ "(" ~ (identifier | string_literal) ~ ")" }
234234
// ---------------------------------------------------------------------
235235
math_function_call = { math_function_name ~ "(" ~ function_args? ~ ")" }
236236
function_args = { math_expression ~ ("," ~ math_expression)* }
237-
math_expression = { math_function_call | evaluates_to_number | anonymous_traversal }
237+
math_expression = { math_function_call | id_traversal | evaluates_to_number | anonymous_traversal }
238238

239239
math_function_name = {
240240
// Arithmetic (binary)

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -405,10 +405,20 @@ fn validate_property_access<'a>(
405405
FieldValueType::Expression(expr) => {
406406
// Check if this expression contains a traversal
407407
use crate::helixc::analyzer::methods::traversal_validation::validate_traversal;
408-
use crate::helixc::generator::traversal_steps::NestedTraversalInfo;
408+
use crate::helixc::generator::traversal_steps::{ComputedExpressionInfo, NestedTraversalInfo};
409409
use crate::helixc::parser::types::ExpressionType;
410410

411-
if let ExpressionType::Traversal(tr) = &expr.expr {
411+
if let ExpressionType::MathFunctionCall(_) = &expr.expr {
412+
// Math function call - store as computed expression
413+
gen_traversal.computed_expressions.insert(
414+
field_addition.key.clone(),
415+
ComputedExpressionInfo {
416+
field_name: field_addition.key.clone(),
417+
expression: Box::new(expr.clone()),
418+
},
419+
);
420+
gen_traversal.object_fields.push(field_addition.key.clone());
421+
} else if let ExpressionType::Traversal(tr) = &expr.expr {
412422
// Nested traversal within expression - validate it
413423
let mut nested_gen_traversal =
414424
crate::helixc::generator::traversal_steps::Traversal::default();

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

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -144,16 +144,16 @@ fn build_return_fields(
144144
None
145145
};
146146

147-
// If has_object_step, only add implicit fields if they're explicitly selected
147+
// If has_object_step, only add implicit fields if they're explicitly selected OR has_spread
148148
// Otherwise, add all implicit fields (default behavior)
149149
let should_add_field = |field_name: &str| {
150150
// Exclude if field is in excluded_fields
151151
if traversal.excluded_fields.contains(&field_name.to_string()) {
152152
return false;
153153
}
154-
// If has object step, only include if explicitly selected (possibly with remapping)
154+
// If has object step, only include if explicitly selected (possibly with remapping) OR has_spread
155155
if traversal.has_object_step {
156-
find_output_for_property(field_name).is_some()
156+
find_output_for_property(field_name).is_some() || traversal.has_spread
157157
} else {
158158
true
159159
}
@@ -176,8 +176,8 @@ fn build_return_fields(
176176
RustFieldType::Primitive(GenRef::RefLT("a", RustType::Str)),
177177
));
178178
}
179-
} else if !traversal.has_object_step {
180-
// No object step means return all fields
179+
} else if !traversal.has_object_step || traversal.has_spread {
180+
// No object step or has spread means return all fields
181181
fields.push(ReturnFieldInfo::new_implicit(
182182
"id".to_string(),
183183
RustFieldType::Primitive(GenRef::RefLT("a", RustType::Str)),
@@ -198,7 +198,7 @@ fn build_return_fields(
198198
RustFieldType::Primitive(GenRef::RefLT("a", RustType::Str)),
199199
));
200200
}
201-
} else if !traversal.has_object_step {
201+
} else if !traversal.has_object_step || traversal.has_spread {
202202
fields.push(ReturnFieldInfo::new_implicit(
203203
"label".to_string(),
204204
RustFieldType::Primitive(GenRef::RefLT("a", RustType::Str)),
@@ -222,7 +222,7 @@ fn build_return_fields(
222222
RustFieldType::Primitive(GenRef::RefLT("a", RustType::Str)),
223223
));
224224
}
225-
} else if !traversal.has_object_step {
225+
} else if !traversal.has_object_step || traversal.has_spread {
226226
fields.push(ReturnFieldInfo::new_implicit(
227227
"from_node".to_string(),
228228
RustFieldType::Primitive(GenRef::RefLT("a", RustType::Str)),
@@ -243,7 +243,7 @@ fn build_return_fields(
243243
RustFieldType::Primitive(GenRef::RefLT("a", RustType::Str)),
244244
));
245245
}
246-
} else if !traversal.has_object_step {
246+
} else if !traversal.has_object_step || traversal.has_spread {
247247
fields.push(ReturnFieldInfo::new_implicit(
248248
"to_node".to_string(),
249249
RustFieldType::Primitive(GenRef::RefLT("a", RustType::Str)),
@@ -265,7 +265,7 @@ fn build_return_fields(
265265
RustFieldType::RefArray(RustType::F64),
266266
));
267267
}
268-
} else if !traversal.has_object_step {
268+
} else if !traversal.has_object_step || traversal.has_spread {
269269
fields.push(ReturnFieldInfo::new_implicit(
270270
"data".to_string(),
271271
RustFieldType::RefArray(RustType::F64),
@@ -286,7 +286,7 @@ fn build_return_fields(
286286
RustFieldType::Primitive(GenRef::Std(RustType::F64)),
287287
));
288288
}
289-
} else if !traversal.has_object_step {
289+
} else if !traversal.has_object_step || traversal.has_spread {
290290
fields.push(ReturnFieldInfo::new_implicit(
291291
"score".to_string(),
292292
RustFieldType::Primitive(GenRef::Std(RustType::F64)),
@@ -321,6 +321,11 @@ fn build_return_fields(
321321
continue;
322322
}
323323

324+
// Skip if it's a computed expression (handled separately)
325+
if traversal.computed_expressions.contains_key(field_name) {
326+
continue;
327+
}
328+
324329
// Look up the actual property name from the mapping
325330
let property_name = traversal
326331
.field_name_mappings
@@ -734,6 +739,17 @@ fn build_return_fields(
734739
}
735740
}
736741

742+
// Step 4: Add computed expression fields
743+
for (field_name, computed_info) in &traversal.computed_expressions {
744+
fields.push(ReturnFieldInfo {
745+
name: field_name.clone(),
746+
field_type: ReturnFieldType::Simple(RustFieldType::Value),
747+
source: ReturnFieldSource::ComputedExpression {
748+
expression: computed_info.expression.clone(),
749+
},
750+
});
751+
}
752+
737753
fields
738754
}
739755

@@ -1004,6 +1020,7 @@ fn process_object_literal<'a>(
10041020
aggregate_properties: Vec::new(),
10051021
is_count_aggregate: false,
10061022
closure_param_name: None,
1023+
primitive_literal_value: None,
10071024
}
10081025
}
10091026

@@ -1336,7 +1353,7 @@ fn analyze_return_expr<'a>(
13361353
ReturnValue {
13371354
name: rust_type,
13381355
fields,
1339-
literal_value,
1356+
literal_value: literal_value.clone(),
13401357
},
13411358
));
13421359

@@ -1349,6 +1366,7 @@ fn analyze_return_expr<'a>(
13491366
let mut prim_struct = ReturnValueStruct::new(field_name.clone());
13501367
prim_struct.source_variable = field_name.clone();
13511368
prim_struct.is_primitive = true;
1369+
prim_struct.primitive_literal_value = literal_value;
13521370
query.return_structs.push(prim_struct);
13531371
} else {
13541372
let struct_name_prefix = format!(

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

Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,221 @@
1+
// Copyright 2025 HelixDB Inc.
2+
// SPDX-License-Identifier: AGPL-3.0
3+
4+
//! Code generation for computed expression fields in RETURN statements.
5+
//!
6+
//! Handles expressions like `ADD(_::Out<HasRailwayCluster>::COUNT, _::Out<HasObjectCluster>::COUNT)`
7+
//! in object projections.
8+
//!
9+
//! Since `Value` implements standard math ops (`Add`, `Sub`, `Mul`, `Div`), the generated
10+
//! code can directly use operators on `Value` types without conversion.
11+
12+
use crate::helixc::parser::types::{Expression, ExpressionType, MathFunction};
13+
14+
/// Generate Rust code for a computed expression field.
15+
///
16+
/// This handles math function calls (ADD, SUB, MUL, etc.) where arguments
17+
/// may be traversal expressions (like `_::Out<HasCluster>::COUNT`).
18+
///
19+
/// # Arguments
20+
/// * `expression` - The parsed expression to generate code for
21+
/// * `item_var` - The variable name for the current item (e.g., "cluster" in a collection iteration)
22+
///
23+
/// # Returns
24+
/// Rust code string that evaluates to a `Value`
25+
pub fn generate_computed_expression(expression: &Expression, item_var: &str) -> String {
26+
match &expression.expr {
27+
ExpressionType::MathFunctionCall(call) => {
28+
// Generate code for each argument recursively
29+
let args: Vec<String> = call
30+
.args
31+
.iter()
32+
.map(|arg| generate_computed_expression(arg, item_var))
33+
.collect();
34+
35+
match &call.function {
36+
MathFunction::Add => {
37+
if args.len() == 2 {
38+
format!("({}) + ({})", args[0], args[1])
39+
} else {
40+
"Value::Empty".to_string()
41+
}
42+
}
43+
MathFunction::Sub => {
44+
if args.len() == 2 {
45+
format!("({}) - ({})", args[0], args[1])
46+
} else {
47+
"Value::Empty".to_string()
48+
}
49+
}
50+
MathFunction::Mul => {
51+
if args.len() == 2 {
52+
format!("({}) * ({})", args[0], args[1])
53+
} else {
54+
"Value::Empty".to_string()
55+
}
56+
}
57+
MathFunction::Div => {
58+
if args.len() == 2 {
59+
format!("({}) / ({})", args[0], args[1])
60+
} else {
61+
"Value::Empty".to_string()
62+
}
63+
}
64+
MathFunction::Pow => {
65+
// pow is not implemented via traits, need special handling
66+
if args.len() == 2 {
67+
format!("({}).pow(&({}))", args[0], args[1])
68+
} else {
69+
"Value::Empty".to_string()
70+
}
71+
}
72+
MathFunction::Mod => {
73+
if args.len() == 2 {
74+
format!("({}) % ({})", args[0], args[1])
75+
} else {
76+
"Value::Empty".to_string()
77+
}
78+
}
79+
MathFunction::Abs => {
80+
if args.len() == 1 {
81+
format!("({}).abs()", args[0])
82+
} else {
83+
"Value::Empty".to_string()
84+
}
85+
}
86+
MathFunction::Sqrt => {
87+
if args.len() == 1 {
88+
format!("({}).sqrt()", args[0])
89+
} else {
90+
"Value::Empty".to_string()
91+
}
92+
}
93+
MathFunction::Min => {
94+
if args.len() == 2 {
95+
format!("({}).min(({}))", args[0], args[1])
96+
} else {
97+
"Value::Empty".to_string()
98+
}
99+
}
100+
MathFunction::Max => {
101+
if args.len() == 2 {
102+
format!("({}).max(({}))", args[0], args[1])
103+
} else {
104+
"Value::Empty".to_string()
105+
}
106+
}
107+
_ => "Value::Empty /* unsupported math function */".to_string(),
108+
}
109+
}
110+
ExpressionType::Traversal(traversal) => {
111+
// Generate traversal code that returns a Value directly
112+
generate_traversal_value(traversal, item_var)
113+
}
114+
ExpressionType::IntegerLiteral(val) => {
115+
format!("Value::from({})", val)
116+
}
117+
ExpressionType::FloatLiteral(val) => {
118+
format!("Value::from({})", val)
119+
}
120+
ExpressionType::Identifier(id) => {
121+
// Identifier - could be a variable reference that's already a Value
122+
id.clone()
123+
}
124+
_ => "Value::Empty".to_string(),
125+
}
126+
}
127+
128+
/// Generate traversal code that returns a Value.
129+
fn generate_traversal_value(
130+
traversal: &crate::helixc::parser::types::Traversal,
131+
item_var: &str,
132+
) -> String {
133+
use crate::helixc::parser::types::{GraphStepType, StartNode, StepType};
134+
135+
// Check if this starts with anonymous (_)
136+
let source_var = match &traversal.start {
137+
StartNode::Anonymous => item_var.to_string(),
138+
StartNode::Identifier(id) => id.clone(),
139+
_ => item_var.to_string(),
140+
};
141+
142+
// Check for direct property access pattern:
143+
// - Exactly one step
144+
// - That step is StepType::Object with a single field
145+
// - No graph traversal steps (Out/In/OutE/InE)
146+
// This generates direct property access like `item.get_property("price")`
147+
// instead of wrapping in G::from_iter()
148+
if traversal.steps.len() == 1
149+
&& let StepType::Object(obj) = &traversal.steps[0].step
150+
&& obj.fields.len() == 1
151+
{
152+
let field = &obj.fields[0];
153+
let prop_name = &field.key;
154+
if prop_name == "id" || prop_name == "ID" {
155+
return format!("Value::from(uuid_str({}.id(), &arena))", source_var);
156+
} else if prop_name == "label" || prop_name == "Label" {
157+
return format!("Value::from({}.label())", source_var);
158+
} else {
159+
return format!(
160+
"{}.get_property(\"{}\").expect(\"property not found\").clone()",
161+
source_var, prop_name
162+
);
163+
}
164+
}
165+
166+
// Build the traversal steps for graph traversals
167+
let mut steps = String::new();
168+
let mut ends_with_count = false;
169+
170+
for step_info in &traversal.steps {
171+
match &step_info.step {
172+
StepType::Node(graph_step) => match &graph_step.step {
173+
GraphStepType::Out(label) => {
174+
steps.push_str(&format!(".out_node(\"{}\")", label));
175+
}
176+
GraphStepType::In(label) => {
177+
steps.push_str(&format!(".in_node(\"{}\")", label));
178+
}
179+
GraphStepType::OutE(label) => {
180+
steps.push_str(&format!(".out_e(\"{}\")", label));
181+
}
182+
GraphStepType::InE(label) => {
183+
steps.push_str(&format!(".in_e(\"{}\")", label));
184+
}
185+
_ => {}
186+
},
187+
StepType::Count => {
188+
steps.push_str(".count_to_val()");
189+
ends_with_count = true;
190+
}
191+
StepType::Object(obj) => {
192+
// Property access
193+
if let Some(field) = obj.fields.first() {
194+
let prop_name = &field.key;
195+
if prop_name == "id" || prop_name == "ID" {
196+
steps.push_str(
197+
".map(|item| item.map(|v| Value::from(uuid_str(v.id(), &arena))))",
198+
);
199+
} else {
200+
steps.push_str(&format!(".get_property(\"{}\")", prop_name));
201+
}
202+
}
203+
}
204+
_ => {}
205+
}
206+
}
207+
208+
if ends_with_count {
209+
// COUNT traversals return Value directly
210+
format!(
211+
"G::from_iter(&db, &txn, std::iter::once({}.clone()), &arena){}",
212+
source_var, steps
213+
)
214+
} else {
215+
// Regular traversal - collect to value
216+
format!(
217+
"G::from_iter(&db, &txn, std::iter::once({}.clone()), &arena){}.collect_to_value()",
218+
source_var, steps
219+
)
220+
}
221+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ use std::io::Write;
2222
use std::{fmt::Display, fs::File, io::Result, path::Path};
2323

2424
pub mod bool_ops;
25+
pub mod computed_expr;
2526
pub mod math_functions;
2627
pub mod migrations;
2728
pub mod queries;

0 commit comments

Comments
 (0)