refactor(cypher): single scalar/aggregate dispatch — unify the 3-way evaluator split#653
Merged
Conversation
…evaluator split 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).
coseto6125
enabled auto-merge (squash)
July 22, 2026 17:47
…gate computed bindings 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.
…rs/edge_vars The prior computed-fallback fix (8d4c5c6) 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.
Contributor
ecp impact cache (34 symbols) — internal, used by
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Collapses the cypher executor's split extension point for adding a scalar function or aggregate:
AggregateKindenum, parsed once viaAggregateKind::parse, replaces the two independently-maintained aggregate name tables (is_aggregate_fn'smatches!list andAccumulator::new'smatch).Accumulator::newnow matches exhaustively on the enum, so a new aggregate without a matching accumulator arm is a compile error instead of the old_ => Counter(0)silent misroute.eval_scalar_funcall) is now used by all three evaluators.eval_return_exprandeval_return_item_rich— previously near-duplicates differing only in whether a boundVarcollapses toStror staysNodeRef/EdgeRef— are now oneeval_return_expr_withparameterized by aVarCollapseenum.eval_expr'sFunCallarm previously rejected all function calls in WHERE outright; it now routes non-aggregate calls through the sharedeval_scalar_funcall, soWHERE TYPE(r) = 'Calls',WHERE id(n) = 5,'Function' IN labels(n)etc. work in WHERE the same as in RETURN. Aggregates in WHERE (WHERE count(n) > 1) still error explicitly — no single-row meaning, matching OpenCypher's own restriction there.Why
Adding a function previously meant remembering three eval sites plus two aggregate string tables; missing one silently misrouted to
Counter(0)or hit theunreachable!at the oldwith_agg_specsconstruction. From the 2026-07-23 architecture review (C3).Compatibility
Zero change to the public
cypher::execute/cypher::parseinterface. Zero output-format change for any existing query shape —VarCollapse::ToStr/ToRefreproduce the oldeval_return_expr/eval_return_item_richbehavior exactly, including the pre-existing quirk where the plain-projection path never checkededge_varsfor an unbound-node var (preserved via thecollapse == ToRefgate).Test plan
#[test]fns in the file are unmodified and green.exec_where_type_of_edge_filters_correctly/_mismatch_returns_empty,exec_where_labels_of_node_filters_correctly— WHERE fn-calls now work.exec_where_aggregate_funcall_is_rejected— aggregate-in-WHERE still errors explicitly.scalar_funcall_identical_across_where_return_and_with— sameTYPE(r)call through WHERE, RETURN, and WITH group-key paths asserts identical result rows.aggregate_kind_every_variant_builds_a_working_accumulator— everyAggregateKindvariant produces a non-Null accumulator after one fed row (pins exhaustiveness).aggregate_kind_parse_matches_is_aggregate_fn_for_all_known_names— classification consistency.cargo test -p ecp-core --lib cypher: 181 passed, 0 failed.cargo test -p egent-code-plexus --tests: 1249 passed, 0 failed (full CLI suite, cypher round-trips through it).cargo clippy -p egent-code-plexus --testsandcargo clippy -p ecp-analyzer: clean (also re-verified by the pre-push hook).rustfmt --edition 2021on the touched file.