fix(cypher): group aggregations by evaluated scalar-function keys - #1279
fix(cypher): group aggregations by evaluated scalar-function keys#1279mustafaabidali wants to merge 1 commit into
Conversation
cbm_return_item_t.func is populated by both aggregate functions (COUNT/SUM/AVG/MIN/MAX/COLLECT) and non-aggregate scalar functions (labels, type, id, keys, properties, toLower, ...). The RETURN and WITH aggregation paths discriminated grouping items on `item->func != NULL` instead of is_aggregate_func(), so any scalar-function group key was (a) excluded from the GROUP BY key and (b) rendered through the aggregate formatter, whose fallback prints the accumulation count. MATCH (n) RETURN labels(n) AS l, count(n) AS c therefore returned one row ["5097","5097"] (node count in both columns) instead of one row per label. Same defect for type(r), toLower(...), and any other scalar function, in both RETURN and WITH paths. This violated the documented contract that unsupported constructs fail loudly rather than silently returning wrong results (see issue DeusData#373 test), and the results were silently wrong rather than obviously so. Fix: switch seven grouping sites to is_aggregate_func(), and in the WITH path evaluate group values through project_item() (as the RETURN path already did) instead of binding_get_virtual(), so computed keys like labels(n) group by their evaluated value. The bare-node-group-var carry (group_node_ids) is narrowed to items with no func/kase/args: computed expressions have no node identity to carry, and without the narrowing a scalar-function alias would inherit the first group member's node id and resolve property access to an arbitrary node. Behavioural note: WITH-path group values now pass through the same 512-byte projection buffer as RETURN-path values, so a >511-byte group value (e.g. a bare edge variable's properties_json) is truncated where it previously was not. This aligns WITH-agg with the existing RETURN-agg and execute_with_simple behaviour. Also adds the DeusData#601 wall-clock guard to execute_with_aggregate's grouping loop, matching execute_return_agg; the fix makes each iteration evaluate every non-aggregate expression, so the loop is strictly heavier than before. Tests: five new cases (reproduce-first; four fail before the fix with row_count==1, plain-property control passes before and after). Signed-off-by: mustafaabidali <mustafaabidali1337@gmail.com>
|
Thank you for the contribution and for reproducing the silent wrong-result behavior across both RETURN and WITH aggregation paths. This is now triaged as a high-priority Cypher correctness bug for |
|
A quick note so this does not sit here looking like your problem: your red CI is not caused by your change. Your run hit the Windows launcher error-32 scan window — a staging-path issue that was fixed on One thing worth knowing, because it will save you a wasted click: "Re-run failed jobs" will not clear it. A re-run re-tests the same recorded merge commit and fails identically. Only a fresh push refreshes the merge ref — so a rebase on current Apologies for the delay. We are working through a large review backlog oldest-first, and I did not want you debugging a failure that was ours. Once it is green I will review it properly. |
What does this PR do?
Fixes silent wrong results for any aggregation grouped by a scalar-function key —
labels(n),type(r),toLower(n.prop), etc. — in both the RETURN and WITH paths ofquery_graph.Symptom (real index, 5,097 nodes / 13 labels):
Expected: 13 rows of label→count. Same failure for
type(r)(16 edge types collapsed to["8847","8847"]) and every other scalar function used as a group key.Root cause:
cbm_return_item_t.funcis populated by both aggregates (COUNT/SUM/…) and non-aggregate scalar functions (labels,type,id,keys,properties,toLower, …). Seven grouping sites discriminated onitem->func != NULLinstead ofis_aggregate_func(). A scalar-function item was therefore (a) dropped from the GROUP BY key — collapsing all rows into one group — and (b) rendered throughformat_agg_value, whose fallback prints the accumulation count. Notably, bothhas_agggates already usedis_aggregate_func(); only the grouping internals didn't, which is why the query entered the aggregate executor and then misgrouped. This also sidestepped the project's stated contract (issue #373 test) that unsupported constructs fail loudly rather than silently returning plausible-but-wrong data.Fix:
functruthiness tois_aggregate_func().project_item()(as the RETURN path already did) instead ofbinding_get_virtual(), so computed keys group by their evaluated value. This also fixes CASE expressions as WITH group keys, which previously all grouped under an empty string.group_node_idsbare-node-var carry narrowed to items with nofunc/kase/args— without this, a scalar-function alias (e.g.labels(n) AS l) would inherit the first group member's node id and downstream property access on the alias would resolve to an arbitrary node.query_graphhangs on whole-graphOPTIONAL MATCH(no execution timeout / degradation) on large graphs #601 wall-clock guard toexecute_with_aggregate's grouping loop, matching its twinexecute_return_agg— the fix makes each iteration evaluate every non-aggregate expression, so the loop is strictly heavier than before.Behavioural disclosure: WITH-path group values now pass through the same 512-byte projection buffer as RETURN-path values. A >511-byte group value (e.g. a bare edge variable's
properties_json) is truncated where it previously was not, and two values differing only after byte 511 now merge into one group. This aligns WITH-agg with the existingret_agg_build_keyandexecute_with_simplebehaviour rather than introducing a new limit, but it is an observable change on that path.Tests (reproduce-first): five new cases in
tests/test_cypher.c—labels()/type()/toLower()group keys in RETURN,labels()in WITH, and a plain-property control. Before the fix the four bug cases fail withrow_count == 1; the control passes before and after. Full suite: 6,791 passed / 4 skipped / 0 failed. Verified on a real 5k-node index:labels()yields 13 rows summing exactly to the node count,type()yields 16 rows summing to the edge count.Possible follow-up (out of scope here): the underlying trap is that one
funcfield carries aggregates, scalar funcs, string funcs, and multi-arg funcs, so every consumer must re-classify. A parse-time discriminator (enum oris_aggflag oncbm_return_item_t) would make this bug class unrepresentable.Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)