fix(cypher): classify aggregation columns by is_aggregate_func, not a bare func check - #1221
Conversation
… bare func check RETURN/WITH aggregation classified each projected column with a bare `if (item->func)` check to decide group-key vs. aggregate value. That check is true for any function call, so type()/labels()/id()/keys()/ properties() were routed into the aggregate branch alongside real aggregates and formatted via format_agg_value's default case, which emits the row count instead of the function's actual value. Swap the five call sites (ret_agg_build_key, ret_agg_emit_row, with_agg_build_key, with_agg_find_or_create, with_agg_accumulate) to is_aggregate_func(), the predicate already used correctly one level up to decide whether a query needs aggregation at all. Group-key columns carrying a non-aggregate function now project through the existing project_item() helper instead of binding_get_virtual(), since they need to evaluate the function rather than read a raw property. Fixes DeusData#1111 (the type(r)/count(*) half; the inline-property-map half of that issue does not reproduce on current HEAD) Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
|
Thanks, this is focused and the reported RETURN/WITH RED cases are good. One correctness regression remains before approval: scalar node functions now reach the existing bare-node carry path. |
… id carry The bare-node-carry check in with_agg_find_or_create only tested !property && variable, so labels(n)/id(n)/keys(n)/properties(n) aliases (variable set, property NULL, func set) were also tagged with the source node's id. A later alias.property then hit node_prop's stub re-fetch heuristic and silently returned the source node's real property instead of empty for the non-node alias. Adds cypher_issue1111_with_scalar_func_alias_no_node_leak, which fails on the prior code (returns the source node's file_path) and passes with this fix. Addresses DeusData's review on DeusData#1221.
|
Good catch, fixed in 5fb4c49. Excluded items with |
… id carry The bare-node-carry check in with_agg_find_or_create only tested !property && variable, so labels(n)/id(n)/keys(n)/properties(n) aliases (variable set, property NULL, func set) were also tagged with the source node's id. A later alias.property then hit node_prop's stub re-fetch heuristic and silently returned the source node's real property instead of empty for the non-node alias. Adds cypher_issue1111_with_scalar_func_alias_no_node_leak, which fails on the prior code (returns the source node's file_path) and passes with this fix. Addresses DeusData's review on DeusData#1221. Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
5fb4c49 to
c218e0e
Compare
|
The |
What does this PR do?
Fixes the
type(r)/count(*)half of #1111:RETURN/WITHaggregation misroutes non-aggregate functions into the aggregate-value branch whenever the query also carries an aggregate column, silently substituting the row count for the actual value.Root cause
ret_agg_build_key/ret_agg_emit_row(RETURN) andwith_agg_build_key/with_agg_find_or_create/with_agg_accumulate(WITH) classify each projected column with a bareif (item->func)check to decide "is this the aggregate column or the group-key column". That check is wrong:item->funcis set for any function call, aggregate or not, sotype(r),labels(n),id(n),keys(n), andproperties(n)read as truthy and land in the aggregate branch next to real aggregates likecount(*). There they are formatted byformat_agg_value's default case, which emits the accumulated row count, not the value the query asked for.The correct predicate already exists and is already used correctly one level up, where
execute_return_clause/execute_with_clausedecide whether a query needs aggregation at all:is_aggregate_func(), which checks the function name against the fixed aggregate set (COUNT/SUM/AVG/MIN/MAX/COLLECT). The per-column classification inside the aggregation-execution path never adopted it.The fix
Swap the five
item->functruthy checks foris_aggregate_func(item->func). A group-key column that carries a non-aggregate function now needs to actually evaluate that function instead of reading a raw variable/property, so its value comes from the existingproject_item()helper (already used by the non-aggregated RETURN/WITH paths) rather thanbinding_get_virtual().Verification
Docker (
ubuntu:24.04, gcc, ASan+UBSan build):src/cypher/cypher.chunk with the two new tests kept: both fail exactly as reported (type(r)andcount(*)both return the row count).cyphersuite is 162/162.scripts/lint.sh --ci CLANG_FORMAT=clang-format-20(cppcheck 2.20.0 + clang-format-20, matching_lint.yml): clean.scripts/security-audit.sh: passed, no new findings touching the changed files.make -f Makefile.cbm test-par): 6770 passed, 12 failed, 4 skipped. The 12 failures (insubprocess,cli,mcp,index_supervisor, all process-tree/daemon-lifecycle tests unrelated tocypher.c) reproduce identically on unmodifiedmainin the same container, so they predate this change.Not verified: the other claim in #1111, an inline property map in a
MATCHpattern silently matching 0 rows, does not reproduce on current HEAD and is unrelated to this fix. Leaving that open for you to split or close separately.Fixes #1111