From 69d88d499d19a5ca4ebbae9617aaccbb9d7edf2f Mon Sep 17 00:00:00 2001 From: coseto6125 <80243681+coseto6125@users.noreply.github.com> Date: Thu, 23 Jul 2026 01:46:02 +0800 Subject: [PATCH 1/3] =?UTF-8?q?refactor(cypher):=20single=20scalar/aggrega?= =?UTF-8?q?te=20dispatch=20=E2=80=94=20unify=20the=203-way=20evaluator=20s?= =?UTF-8?q?plit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding a scalar function or aggregate previously meant remembering three evaluator sites (eval_expr for WHERE, eval_return_expr, eval_return_item_rich) and two independently-maintained aggregate name lists (is_aggregate_fn's match!, Accumulator::new's match) — missing one meant a silent Null misroute or an unreachable panic. - AggregateKind enum, parsed once via AggregateKind::parse, replaces the two string tables. Accumulator::new now matches on the enum exhaustively, so a new aggregate without a matching arm is a compile error instead of the old `_ => Counter(0)` silent fallback. - eval_return_expr and eval_return_item_rich collapsed into one eval_return_expr_with, parameterized by VarCollapse (their only point of divergence: whether a bound Var resolves to Str or stays NodeRef/EdgeRef). - WHERE-clause function calls now route through the same eval_scalar_funcall used by RETURN and WITH, so `WHERE TYPE(r) = 'Calls'` etc. work; aggregates in WHERE still error explicitly (no single-row meaning, matching OpenCypher). From the 2026-07-23 architecture review (C3). --- crates/ecp-core/src/cypher/executor.rs | 369 ++++++++++++++++++------- 1 file changed, 274 insertions(+), 95 deletions(-) diff --git a/crates/ecp-core/src/cypher/executor.rs b/crates/ecp-core/src/cypher/executor.rs index ee040a87..4d6187ca 100644 --- a/crates/ecp-core/src/cypher/executor.rs +++ b/crates/ecp-core/src/cypher/executor.rs @@ -385,8 +385,8 @@ fn execute_inner( .collect(); // Identify aggregate positions once so the per-row loop avoids re-scanning. - // Each entry: (expanded_items index, is_count_star, arg expr, name, distinct). - let agg_positions: Vec<(usize, bool, Option<&Expr>, &str, bool)> = expanded_items + // Each entry: (expanded_items index, is_count_star, arg expr, kind, distinct). + let agg_positions: Vec<(usize, bool, Option<&Expr>, AggregateKind, bool)> = expanded_items .iter() .enumerate() .filter_map(|(i, (_, e))| { @@ -396,11 +396,10 @@ fn execute_inner( args, } = e { - if is_aggregate_fn(name) { - let is_cs = matches!(args.as_slice(), [Expr::Lit(Literal::Null)]); - let arg = if is_cs { None } else { args.first() }; - return Some((i, is_cs, arg, name.as_str(), *distinct)); - } + let kind = AggregateKind::parse(name)?; + let is_cs = matches!(args.as_slice(), [Expr::Lit(Literal::Null)]); + let arg = if is_cs { None } else { args.first() }; + return Some((i, is_cs, arg, kind, *distinct)); } None }) @@ -437,7 +436,7 @@ fn execute_inner( let slot = *key_index.entry(key_str).or_insert_with(|| { let accums = agg_positions .iter() - .map(|(_, _, _, name, distinct)| Accumulator::new(name, *distinct)) + .map(|(_, _, _, kind, distinct)| Accumulator::new(*kind, *distinct)) .collect(); groups.push((key_vals.clone(), accums)); groups.len() - 1 @@ -457,7 +456,7 @@ fn execute_inner( if groups.is_empty() && group_items.is_empty() { let accums = agg_positions .iter() - .map(|(_, _, _, name, distinct)| Accumulator::new(name, *distinct)) + .map(|(_, _, _, kind, distinct)| Accumulator::new(*kind, *distinct)) .collect(); groups.push((vec![], accums)); } @@ -687,54 +686,32 @@ fn expand_return_items( Ok(out) } -/// Evaluate a ReturnExpr directly against a binding (used in the non-agg projection path). -fn eval_return_expr( - expr: &ReturnExpr, - b: &Binding, - graph: Gv<'_>, - cache: &mut ContentCache, -) -> Result { - match expr { - ReturnExpr::Prop(var, prop) => Ok(prop_value(var, prop, b, graph, cache)), - ReturnExpr::Var(var) => { - if let Some(v) = b.computed.get(var) { - Ok(v.clone()) - } else if let Some(&idx) = b.node_vars.get(var) { - Ok(match graph.mnode(idx) { - Some(m) => Value::Str(m.name(&graph).into()), - None => Value::Null, - }) - } else { - Ok(Value::Null) - } - } - ReturnExpr::Star => Ok(Value::Null), - ReturnExpr::FunCall { name, args, .. } => { - // Aggregate FunCalls reach this path only when the caller already - // verified there is no aggregate in the projection — so treating - // them as scalar is a safe no-op (returns Null). - Ok(eval_scalar_funcall(name, args, b, graph)) - } - } -} - -/// Stable string key for a Value (used as group-by key; avoids Hash on Value). -fn value_key(v: &Value) -> String { - format!("{v:?}") +/// How a bound `Var` collapses when a `ReturnExpr::Var` is evaluated. The two +/// callers only ever disagree on this one axis — `Prop`/`Star`/`FunCall` are +/// identical either way — so it is the sole parameter distinguishing them. +#[derive(Clone, Copy, PartialEq, Eq)] +enum VarCollapse { + /// Node/edge vars resolve to their display name (`Value::Str`) — used by + /// the plain RETURN projection path, matching legacy scalar semantics. + ToStr, + /// Node/edge vars resolve to `Value::NodeRef`/`Value::EdgeRef`, preserving + /// identity — used by WITH group-key computation so `a.name` still + /// resolves after aggregation clears `node_vars`. + ToRef, } -/// Evaluate a ReturnItem's expression into a Value, preserving NodeRef/EdgeRef -/// for variables bound to graph nodes/edges. Used by WITH group-key computation -/// so that `a.name` still resolves after aggregation clears node_vars. -fn eval_return_item_rich( - item: &ReturnItem, +/// Evaluate a ReturnExpr against a binding. Single dispatch point shared by +/// the plain RETURN projection path and WITH group-key computation; `collapse` +/// selects the one axis where they differ (see `VarCollapse`). +fn eval_return_expr_with( + expr: &ReturnExpr, b: &Binding, graph: Gv<'_>, cache: &mut ContentCache, + collapse: VarCollapse, ) -> Value { - match &item.expr { + match expr { ReturnExpr::Var(var) => { - // Check computed first. if let Some(v) = b.computed.get(var) { return v.clone(); } @@ -742,32 +719,37 @@ fn eval_return_item_rich( let Some(m) = graph.mnode(idx) else { return Value::Null; }; - return Value::NodeRef { - idx, - name: m.name(&graph).into(), - kind: m.kind().as_str().into(), - file_path: m.file_path(&graph).unwrap_or("").to_string(), + return match collapse { + VarCollapse::ToStr => Value::Str(m.name(&graph).into()), + VarCollapse::ToRef => Value::NodeRef { + idx, + name: m.name(&graph).into(), + kind: m.kind().as_str().into(), + file_path: m.file_path(&graph).unwrap_or("").to_string(), + }, }; } - if let Some(&eidx) = b.edge_vars.get(var) { - if let Some(e) = graph.overlay_edge(eidx) { + if collapse == VarCollapse::ToRef { + if let Some(&eidx) = b.edge_vars.get(var) { + if let Some(e) = graph.overlay_edge(eidx) { + return Value::EdgeRef { + src: e.source, + tgt: e.target, + rel_type: e.rel_type, + confidence: e.confidence, + reason: "l1-overlay".to_string(), + }; + } + let e = &graph.edges[eidx as usize]; + let rt = crate::graph::RelType::from(&e.rel_type); return Value::EdgeRef { - src: e.source, - tgt: e.target, - rel_type: e.rel_type, - confidence: e.confidence, - reason: "l1-overlay".to_string(), + src: e.source.to_native(), + tgt: e.target.to_native(), + rel_type: rt, + confidence: e.confidence.to_native(), + reason: e.reason.resolve(&graph.string_pool).to_string(), }; } - let e = &graph.edges[eidx as usize]; - let rt = crate::graph::RelType::from(&e.rel_type); - return Value::EdgeRef { - src: e.source.to_native(), - tgt: e.target.to_native(), - rel_type: rt, - confidence: e.confidence.to_native(), - reason: e.reason.resolve(&graph.string_pool).to_string(), - }; } Value::Null } @@ -777,6 +759,42 @@ fn eval_return_item_rich( } } +/// Evaluate a ReturnExpr directly against a binding (used in the non-agg projection path). +/// Aggregate FunCalls reach this path only when the caller already verified +/// there is no aggregate in the projection — so treating them as scalar is a +/// safe no-op (returns Null via `eval_scalar_funcall`'s unknown-name fallback). +fn eval_return_expr( + expr: &ReturnExpr, + b: &Binding, + graph: Gv<'_>, + cache: &mut ContentCache, +) -> Result { + Ok(eval_return_expr_with( + expr, + b, + graph, + cache, + VarCollapse::ToStr, + )) +} + +/// Stable string key for a Value (used as group-by key; avoids Hash on Value). +fn value_key(v: &Value) -> String { + format!("{v:?}") +} + +/// Evaluate a ReturnItem's expression into a Value, preserving NodeRef/EdgeRef +/// for variables bound to graph nodes/edges. Used by WITH group-key computation +/// so that `a.name` still resolves after aggregation clears node_vars. +fn eval_return_item_rich( + item: &ReturnItem, + b: &Binding, + graph: Gv<'_>, + cache: &mut ContentCache, +) -> Value { + eval_return_expr_with(&item.expr, b, graph, cache, VarCollapse::ToRef) +} + /// Execute a WITH clause: rebind plain items into `computed`, or group+aggregate. fn exec_with( wc: &WithClause, @@ -809,7 +827,7 @@ fn exec_with( .collect(); // Aggregate positions for the WITH clause. - let with_agg_specs: Vec<(String, bool, Option<&Expr>, &str, bool)> = agg_items + let with_agg_specs: Vec<(String, bool, Option<&Expr>, AggregateKind, bool)> = agg_items .iter() .map(|ai| { let col = ai @@ -824,7 +842,9 @@ fn exec_with( { let is_cs = matches!(args.as_slice(), [Expr::Lit(Literal::Null)]); let arg = if is_cs { None } else { args.first() }; - (col, is_cs, arg, name.as_str(), *distinct) + let kind = AggregateKind::parse(name) + .expect("agg_items filtered to is_aggregate_fn names"); + (col, is_cs, arg, kind, *distinct) } else { unreachable!("agg_items filtered to FunCall aggregates") } @@ -855,7 +875,7 @@ fn exec_with( let slot = *key_index.entry(key_str).or_insert_with(|| { let accums = with_agg_specs .iter() - .map(|(_, _, _, name, distinct)| Accumulator::new(name, *distinct)) + .map(|(_, _, _, kind, distinct)| Accumulator::new(*kind, *distinct)) .collect(); groups.push((key_pairs.clone(), accums)); groups.len() - 1 @@ -925,19 +945,51 @@ fn exec_with( Ok(out) } -/// Names recognized as aggregate functions. Anything else parsed as a -/// FunCall is treated as a scalar function (`type(r)`, `id(n)`, `labels(n)`). +/// Single source of truth for which FunCall names are aggregates. Parsed once +/// per FunCall (`AggregateKind::parse`) instead of re-checked via a hardcoded +/// string list at every classification site; `Accumulator::new` then matches +/// on the enum exhaustively, so adding a variant without wiring an accumulator +/// arm is a compile error instead of a silent fallback to `Counter(0)`. /// Pre-uppercased — the parser normalizes (`parser.rs:382/398/572/588`). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum AggregateKind { + Count, + Sum, + Avg, + Min, + Max, + Collect, +} + +impl AggregateKind { + fn parse(name: &str) -> Option { + Some(match name { + "COUNT" => Self::Count, + "SUM" => Self::Sum, + "AVG" => Self::Avg, + "MIN" => Self::Min, + "MAX" => Self::Max, + "COLLECT" => Self::Collect, + _ => return None, + }) + } +} + +/// Anything else parsed as a FunCall is treated as a scalar function +/// (`type(r)`, `id(n)`, `labels(n)`). fn is_aggregate_fn(name: &str) -> bool { - matches!(name, "COUNT" | "SUM" | "AVG" | "MIN" | "MAX" | "COLLECT") + AggregateKind::parse(name).is_some() } -/// Evaluate a scalar (non-aggregate) function call. Returns `Value::Null` for -/// unknown functions rather than erroring — matches the OpenCypher convention -/// that missing-data scalars degrade gracefully (see graph_query rel-type -/// `matches!` path). Supports the three functions LLM agents reach for most: -/// `type(r)` → edge rel-type as Str; `id(n)` → node index as Int; -/// `labels(n)` → single-element list of node-kind Str. +/// Evaluate a scalar (non-aggregate) function call. Single dispatch point +/// shared by WHERE, plain RETURN, and rich-RETURN (WITH group-key) — a new +/// scalar function is wired here once and all three evaluators pick it up. +/// Returns `Value::Null` for unknown functions rather than erroring — +/// matches the OpenCypher convention that missing-data scalars degrade +/// gracefully (see graph_query rel-type `matches!` path). Supports the three +/// functions LLM agents reach for most: `type(r)` → edge rel-type as Str; +/// `id(n)` → node index as Int; `labels(n)` → single-element list of +/// node-kind Str. fn eval_scalar_funcall(name: &str, args: &[Expr], b: &Binding, graph: Gv<'_>) -> Value { match name { "TYPE" => { @@ -1006,31 +1058,32 @@ enum Accumulator { } impl Accumulator { - fn new(name: &str, distinct: bool) -> Self { - match name { - "COUNT" => { + /// Exhaustive over `AggregateKind` — a new variant without a matching arm + /// here is a compile error, not a silent fallback to `Counter(0)`. + fn new(kind: AggregateKind, distinct: bool) -> Self { + match kind { + AggregateKind::Count => { if distinct { Accumulator::CounterDistinct(HashSet::new()) } else { Accumulator::Counter(0) } } - "SUM" => Accumulator::Summer { + AggregateKind::Sum => Accumulator::Summer { sum_i: 0, sum_f: 0.0, has_float: false, }, - "MIN" => Accumulator::MinAccum(None), - "MAX" => Accumulator::MaxAccum(None), - "COLLECT" => { + AggregateKind::Min => Accumulator::MinAccum(None), + AggregateKind::Max => Accumulator::MaxAccum(None), + AggregateKind::Collect => { if distinct { Accumulator::CollectorDistinct(Vec::new(), HashSet::new()) } else { Accumulator::Collector(Vec::new()) } } - "AVG" => Accumulator::Avg { sum: 0.0, count: 0 }, - _ => Accumulator::Counter(0), + AggregateKind::Avg => Accumulator::Avg { sum: 0.0, count: 0 }, } } @@ -1747,9 +1800,17 @@ fn eval_expr( ExistsPattern { pattern, negated } => { Ok(Value::Bool(pattern_exists(pattern, b, graph)? ^ negated)) } - FunCall { .. } => Err(CypherError::Exec { - msg: "function calls in WHERE not yet supported".into(), - }), + FunCall { name, args, .. } => { + // Aggregates have no meaning against a single row's binding — same + // restriction as OpenCypher (`WHERE count(n) > 1` is a semantic + // error there too, not a WHERE-specific gap in this executor). + if is_aggregate_fn(name) { + return Err(CypherError::Exec { + msg: format!("aggregate function {name}() not allowed in WHERE"), + }); + } + Ok(eval_scalar_funcall(name, args, b, graph)) + } } } @@ -2953,6 +3014,124 @@ mod tests { }); } + // ----------------------------------------------------------------------- + // WHERE-clause function calls — enabled by the single scalar dispatch + // (`eval_scalar_funcall`) shared with RETURN and WITH group-key paths. + // ----------------------------------------------------------------------- + + #[test] + fn exec_where_type_of_edge_filters_correctly() { + with_two(|g| { + let q = parse( + "MATCH (a:Function)-[r:Calls]->(b:Function) WHERE TYPE(r) = 'Calls' RETURN a.name", + ) + .unwrap(); + let r = execute(&q, g, None, Path::new(".")).unwrap(); + assert_eq!(r.rows.len(), 1); + }); + } + + #[test] + fn exec_where_type_of_edge_mismatch_returns_empty() { + with_two(|g| { + let q = parse( + "MATCH (a:Function)-[r:Calls]->(b:Function) WHERE TYPE(r) = 'Imports' RETURN a.name", + ) + .unwrap(); + let r = execute(&q, g, None, Path::new(".")).unwrap(); + assert_eq!(r.rows.len(), 0); + }); + } + + #[test] + fn exec_where_labels_of_node_filters_correctly() { + with_two(|g| { + let q = parse( + "MATCH (a:Function) WHERE 'Function' IN labels(a) RETURN a.name ORDER BY a.name", + ) + .unwrap(); + let r = execute(&q, g, None, Path::new(".")).unwrap(); + assert_eq!(r.rows.len(), 2); + }); + } + + #[test] + fn exec_where_aggregate_funcall_is_rejected() { + with_two(|g| { + let q = parse("MATCH (a:Function) WHERE count(a) > 1 RETURN a.name").unwrap(); + let err = execute(&q, g, None, Path::new(".")).unwrap_err(); + assert!( + matches!(&err, CypherError::Exec { msg } if msg.contains("aggregate") && msg.contains("WHERE")), + "expected aggregate-in-WHERE error, got {err:?}" + ); + }); + } + + #[test] + fn scalar_funcall_identical_across_where_return_and_with() { + // Same TYPE(r) call through all three evaluators must agree — + // regression guard for the unified `eval_scalar_funcall` dispatch. + with_two(|g| { + let where_q = parse( + "MATCH (a:Function)-[r:Calls]->(b:Function) WHERE TYPE(r) = 'Calls' RETURN TYPE(r)", + ) + .unwrap(); + let where_r = execute(&where_q, g, None, Path::new(".")).unwrap(); + + let return_q = + parse("MATCH (a:Function)-[r:Calls]->(b:Function) RETURN TYPE(r)").unwrap(); + let return_r = execute(&return_q, g, None, Path::new(".")).unwrap(); + + let with_q = + parse("MATCH (a:Function)-[r:Calls]->(b:Function) WITH TYPE(r) AS t RETURN t") + .unwrap(); + let with_r = execute(&with_q, g, None, Path::new(".")).unwrap(); + + assert_eq!(where_r.rows.len(), 1); + assert_eq!(where_r.rows, return_r.rows); + assert_eq!(where_r.rows, with_r.rows); + assert_eq!(where_r.rows[0][0], Value::Str("Calls".into())); + }); + } + + // ----------------------------------------------------------------------- + // AggregateKind exhaustiveness — every variant must produce a distinct, + // working `Accumulator`. Guards the failure mode the old string-matched + // `Accumulator::new` fallback (`_ => Counter(0)`) could hit silently. + // ----------------------------------------------------------------------- + + #[test] + fn aggregate_kind_every_variant_builds_a_working_accumulator() { + let kinds = [ + AggregateKind::Count, + AggregateKind::Sum, + AggregateKind::Avg, + AggregateKind::Min, + AggregateKind::Max, + AggregateKind::Collect, + ]; + for kind in kinds { + let mut acc = Accumulator::new(kind, false); + acc.feed(Value::Int(1), false); + let out = acc.finalize(); + assert_ne!( + out, + Value::Null, + "{kind:?} accumulator produced Null after feeding one row" + ); + } + } + + #[test] + fn aggregate_kind_parse_matches_is_aggregate_fn_for_all_known_names() { + for name in ["COUNT", "SUM", "AVG", "MIN", "MAX", "COLLECT"] { + assert!(is_aggregate_fn(name)); + assert!(AggregateKind::parse(name).is_some()); + } + assert!(!is_aggregate_fn("TYPE")); + assert!(AggregateKind::parse("TYPE").is_none()); + } + // ----------------------------------------------------------------------- // OPTIONAL MATCH left-join // ----------------------------------------------------------------------- From 8d4c5c68b80dd31f67d6fb08ecba441ca93c9ab3 Mon Sep 17 00:00:00 2001 From: coseto6125 <80243681+coseto6125@users.noreply.github.com> Date: Thu, 23 Jul 2026 01:58:26 +0800 Subject: [PATCH 2/3] fix(cypher): resolve WHERE scalar funcalls through WITH-alias / aggregate computed bindings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit eval_scalar_funcall only looked up node_vars/edge_vars, which WITH rebinding and aggregation both clear in favor of `computed` (holding a NodeRef/EdgeRef instead). A WHERE funcall on a WITH-aliased var or an aggregate-WITH grouping key therefore silently resolved to Null and filtered out rows that should have survived — e.g. `WITH a AS x WHERE ID(x) IS NOT NULL` dropped every row. TYPE/ID/LABELS now fall back to the matching NodeRef/EdgeRef in `computed` when the var isn't in node_vars/edge_vars, mirroring the existing computed-NodeRef recovery already used by the decorator-IN pushdown path a few hundred lines up. No change to non-FunCall WHERE behavior or to the RETURN paths (they were already unaffected, since eval_return_expr_with checks `computed` first regardless). Follow-up to the C3 cypher evaluator unification (PR #653), caught by Codex review. --- crates/ecp-core/src/cypher/executor.rs | 92 ++++++++++++++++++-------- 1 file changed, 64 insertions(+), 28 deletions(-) diff --git a/crates/ecp-core/src/cypher/executor.rs b/crates/ecp-core/src/cypher/executor.rs index 4d6187ca..d84584c0 100644 --- a/crates/ecp-core/src/cypher/executor.rs +++ b/crates/ecp-core/src/cypher/executor.rs @@ -991,42 +991,49 @@ fn is_aggregate_fn(name: &str) -> bool { /// `id(n)` → node index as Int; `labels(n)` → single-element list of /// node-kind Str. fn eval_scalar_funcall(name: &str, args: &[Expr], b: &Binding, graph: Gv<'_>) -> Value { + let Some(Expr::Var(var)) = args.first() else { + return Value::Null; + }; match name { "TYPE" => { - // type(r) — args[0] must be a Var bound to an edge. - let Some(Expr::Var(var)) = args.first() else { - return Value::Null; - }; - let Some(&eidx) = b.edge_vars.get(var) else { - return Value::Null; - }; - if let Some(e) = graph.overlay_edge(eidx) { - return Value::Str(e.rel_type.as_str().into()); + // type(r) — args[0] must be a Var bound to an edge, either + // directly (edge_vars) or via a WITH alias / aggregate grouping + // key that stashed the resolved EdgeRef in `computed` (WITH + // rebinding and aggregation both clear node_vars/edge_vars). + if let Some(&eidx) = b.edge_vars.get(var) { + if let Some(e) = graph.overlay_edge(eidx) { + return Value::Str(e.rel_type.as_str().into()); + } + let e = &graph.edges[eidx as usize]; + return Value::Str(RelType::from(&e.rel_type).as_str().into()); + } + match b.computed.get(var) { + Some(Value::EdgeRef { rel_type, .. }) => Value::Str(rel_type.as_str().into()), + _ => Value::Null, } - let e = &graph.edges[eidx as usize]; - Value::Str(RelType::from(&e.rel_type).as_str().into()) } "ID" => { - // id(n) — args[0] must be a Var bound to a node. - let Some(Expr::Var(var)) = args.first() else { - return Value::Null; - }; - let Some(&idx) = b.node_vars.get(var) else { - return Value::Null; - }; - Value::Int(idx as i64) + // id(n) — args[0] must be a Var bound to a node, directly or via + // a `computed` NodeRef (same WITH-alias / aggregation gap as TYPE). + if let Some(&idx) = b.node_vars.get(var) { + return Value::Int(idx as i64); + } + match b.computed.get(var) { + Some(Value::NodeRef { idx, .. }) => Value::Int(*idx as i64), + _ => Value::Null, + } } "LABELS" => { // labels(n) — single-kind list per ecp's one-label-per-node model. - let Some(Expr::Var(var)) = args.first() else { - return Value::Null; - }; - let Some(&idx) = b.node_vars.get(var) else { - return Value::Null; - }; - match graph.mnode(idx) { - Some(m) => Value::List(vec![Value::Str(m.kind().as_str().into())]), - None => Value::Null, + if let Some(&idx) = b.node_vars.get(var) { + return match graph.mnode(idx) { + Some(m) => Value::List(vec![Value::Str(m.kind().as_str().into())]), + None => Value::Null, + }; + } + match b.computed.get(var) { + Some(Value::NodeRef { kind, .. }) => Value::List(vec![Value::Str(kind.clone())]), + _ => Value::Null, } } _ => Value::Null, @@ -3094,6 +3101,35 @@ mod tests { }); } + #[test] + fn exec_where_funcall_on_with_alias_preserves_rows() { + // Plain (non-aggregate) WITH rebinding clears node_vars/edge_vars and + // stashes a NodeRef in `computed` instead — a WHERE funcall on the + // aliased var must recover identity from there, not just node_vars. + with_two(|g| { + let q = parse("MATCH (a:Function) WITH a AS x WHERE ID(x) IS NOT NULL RETURN x.name") + .unwrap(); + let r = execute(&q, g, None, Path::new(".")).unwrap(); + assert_eq!(r.rows.len(), 2, "both nodes must survive ID(x) IS NOT NULL"); + }); + } + + #[test] + fn exec_where_funcall_on_aggregate_with_grouping_key_resolves() { + // Aggregating WITH also clears node_vars/edge_vars; the grouping-key + // var (`a`) must still resolve through `computed` for TYPE(r)/ID(a). + with_two(|g| { + let q = parse( + "MATCH (a:Function)-[r:Calls]->(b:Function) WITH a, r, COUNT(b) AS n WHERE TYPE(r) = 'Calls' RETURN a.name, n", + ) + .unwrap(); + let r = execute(&q, g, None, Path::new(".")).unwrap(); + assert_eq!(r.rows.len(), 1); + assert_eq!(r.rows[0][0], Value::Str("caller".into())); + assert_eq!(r.rows[0][1], Value::Int(1)); + }); + } + // ----------------------------------------------------------------------- // AggregateKind exhaustiveness — every variant must produce a distinct, // working `Accumulator`. Guards the failure mode the old string-matched From 2f705744eb4618090d9960e165c8bb2816b40c99 Mon Sep 17 00:00:00 2001 From: coseto6125 <80243681+coseto6125@users.noreply.github.com> Date: Thu, 23 Jul 2026 02:06:42 +0800 Subject: [PATCH 3/3] fix(cypher): scalar funcall lookup must check computed before node_vars/edge_vars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior computed-fallback fix (8d4c5c68) checked node_vars/edge_vars first and only consulted `computed` when the var was entirely absent from them. That order is wrong whenever a WITH clause shadows a surviving variable name, e.g. `WITH b AS a` — exec_with's plain-rebind branch deliberately preserves node_vars/edge_vars unchanged (so a later MATCH can still traverse from the original bindings), so the OLD node_vars["a"] stays present alongside the NEW computed["a"]. TYPE/ID/ LABELS on `a` after such a WITH resolved against the stale pre-WITH entity instead of the one every other evaluator (prop_value, eval_return_expr_with) already reads. Flip the order: check `computed` first whenever the var is present there at all, matching prop_value's existing precedence, and only fall back to node_vars/edge_vars when the var isn't in `computed`. Follow-up to the C3 cypher evaluator unification (PR #653), caught by Codex review. --- crates/ecp-core/src/cypher/executor.rs | 94 +++++++++++++++++--------- 1 file changed, 62 insertions(+), 32 deletions(-) diff --git a/crates/ecp-core/src/cypher/executor.rs b/crates/ecp-core/src/cypher/executor.rs index d84584c0..be778ae5 100644 --- a/crates/ecp-core/src/cypher/executor.rs +++ b/crates/ecp-core/src/cypher/executor.rs @@ -994,46 +994,48 @@ fn eval_scalar_funcall(name: &str, args: &[Expr], b: &Binding, graph: Gv<'_>) -> let Some(Expr::Var(var)) = args.first() else { return Value::Null; }; + // `computed` takes precedence over node_vars/edge_vars whenever the var + // is present there at all — same order `prop_value` uses. A WITH clause + // that shadows a surviving name (`WITH b AS a`) only overwrites + // `computed["a"]`; the plain-rebind branch of `exec_with` deliberately + // preserves node_vars/edge_vars unchanged for downstream MATCH traversal, + // so an old node_vars["a"] can still be sitting there stale. Falling + // back to it after a `computed` miss would resolve the funcall against + // the wrong entity — the pre-WITH one — while every other evaluator + // (prop_value, eval_return_expr_with) already reads the new binding. + if let Some(computed_val) = b.computed.get(var) { + return match (name, computed_val) { + ("TYPE", Value::EdgeRef { rel_type, .. }) => Value::Str(rel_type.as_str().into()), + ("ID", Value::NodeRef { idx, .. }) => Value::Int(*idx as i64), + ("LABELS", Value::NodeRef { kind, .. }) => Value::List(vec![Value::Str(kind.clone())]), + _ => Value::Null, + }; + } match name { "TYPE" => { - // type(r) — args[0] must be a Var bound to an edge, either - // directly (edge_vars) or via a WITH alias / aggregate grouping - // key that stashed the resolved EdgeRef in `computed` (WITH - // rebinding and aggregation both clear node_vars/edge_vars). - if let Some(&eidx) = b.edge_vars.get(var) { - if let Some(e) = graph.overlay_edge(eidx) { - return Value::Str(e.rel_type.as_str().into()); - } - let e = &graph.edges[eidx as usize]; - return Value::Str(RelType::from(&e.rel_type).as_str().into()); - } - match b.computed.get(var) { - Some(Value::EdgeRef { rel_type, .. }) => Value::Str(rel_type.as_str().into()), - _ => Value::Null, + let Some(&eidx) = b.edge_vars.get(var) else { + return Value::Null; + }; + if let Some(e) = graph.overlay_edge(eidx) { + return Value::Str(e.rel_type.as_str().into()); } + let e = &graph.edges[eidx as usize]; + Value::Str(RelType::from(&e.rel_type).as_str().into()) } "ID" => { - // id(n) — args[0] must be a Var bound to a node, directly or via - // a `computed` NodeRef (same WITH-alias / aggregation gap as TYPE). - if let Some(&idx) = b.node_vars.get(var) { - return Value::Int(idx as i64); - } - match b.computed.get(var) { - Some(Value::NodeRef { idx, .. }) => Value::Int(*idx as i64), - _ => Value::Null, - } + let Some(&idx) = b.node_vars.get(var) else { + return Value::Null; + }; + Value::Int(idx as i64) } "LABELS" => { // labels(n) — single-kind list per ecp's one-label-per-node model. - if let Some(&idx) = b.node_vars.get(var) { - return match graph.mnode(idx) { - Some(m) => Value::List(vec![Value::Str(m.kind().as_str().into())]), - None => Value::Null, - }; - } - match b.computed.get(var) { - Some(Value::NodeRef { kind, .. }) => Value::List(vec![Value::Str(kind.clone())]), - _ => Value::Null, + let Some(&idx) = b.node_vars.get(var) else { + return Value::Null; + }; + match graph.mnode(idx) { + Some(m) => Value::List(vec![Value::Str(m.kind().as_str().into())]), + None => Value::Null, } } _ => Value::Null, @@ -3130,6 +3132,34 @@ mod tests { }); } + #[test] + fn exec_where_funcall_on_shadowing_with_alias_uses_new_binding() { + // `WITH b AS a` shadows the surviving `a` name: computed["a"] becomes + // the new binding (callee, idx 1), but the plain-rebind branch of + // exec_with deliberately preserves the OLD node_vars["a"] (caller, + // idx 0) unchanged for downstream MATCH traversal. `ID(a) = 1` only + // stays true if the funcall resolves against the shadowed (new) + // binding — a lookup-order bug that falls back to node_vars first + // would evaluate `ID(a)` as 0, filtering the row out entirely, while + // RETURN a.name (via prop_value, which already checks computed + // first) would still project "callee" had the row survived — i.e. + // the bug manifests as an incorrectly EMPTY result here, not a + // wrong-but-present value. + with_two(|g| { + let q = parse( + "MATCH (a:Function)-[r:Calls]->(b:Function) WITH b AS a WHERE ID(a) = 1 RETURN a.name", + ) + .unwrap(); + let r = execute(&q, g, None, Path::new(".")).unwrap(); + assert_eq!( + r.rows.len(), + 1, + "WHERE ID(a) = 1 must resolve against the shadowed (new) binding, matching what RETURN a.name projects" + ); + assert_eq!(r.rows[0][0], Value::Str("callee".into())); + }); + } + // ----------------------------------------------------------------------- // AggregateKind exhaustiveness — every variant must produce a distinct, // working `Accumulator`. Guards the failure mode the old string-matched