fix(cypher): group aggregations by evaluated scalar-function keys - #1279
Open
mustafaabidali wants to merge 1 commit into
Open
fix(cypher): group aggregations by evaluated scalar-function keys#1279mustafaabidali wants to merge 1 commit into
mustafaabidali 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>
Owner
|
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 |
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
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 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)