trending/codebase-memory-mcp
1
0
Fork 0
mirror of https://github.com/DeusData/codebase-memory-mcp.git synced 2026-11-08 08:53:43 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(cypher): classify aggregation columns by is_aggregate_func, not a bare func check

Browse Source 
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 #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>
This commit is contained in:
Amir Fathi
2026-07-31 06:18:52 +00:00
committed by sleep-agent
parent 8f06867d78
commit b43bdddcbe
2 changed files with 55 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/cypher/cypher.c
+11 -9
View File
@@ -3731,10 +3731,11 @@ typedef struct {
static int with_agg_build_key(cbm_return_clause_t *wc, binding_t *b, char *key, size_t key_sz) {
int kl = 0;
for (int ci = 0; ci < wc->count; ci++) {
if (wc->items[ci].func) {
if (is_aggregate_func(wc->items[ci].func)) {
continue;
}
const char *v = binding_get_virtual(b, wc->items[ci].variable, wc->items[ci].property);
char vbuf[CBM_SZ_512];
const char *v = project_item(b, &wc->items[ci], vbuf, sizeof(vbuf));
kl += snprintf(key + kl, key_sz - (size_t)kl, "%s|", v);
if (kl >= (int)key_sz) {
kl = (int)key_sz - SKIP_ONE;
@@ -3770,11 +3771,12 @@ static int with_agg_find_or_create(with_agg_t **aggs, int *agg_cnt, int *agg_cap
(*aggs)[found].maxs[ci] = -CYP_DBL_MAX;
}
for (int ci = 0; ci < wc->count; ci++) {
if (wc->items[ci].func) {
if (is_aggregate_func(wc->items[ci].func)) {
(*aggs)[found].group_vals[ci] = heap_strdup("0");
continue;
}
const char *v = binding_get_virtual(b, wc->items[ci].variable, wc->items[ci].property);
char vbuf[CBM_SZ_512];
const char *v = project_item(b, &wc->items[ci], vbuf, sizeof(vbuf));
(*aggs)[found].group_vals[ci] = heap_strdup(v);
/* If this group item is a bare node variable, remember its id so the
* carried virtual var can re-fetch any property (group_vals holds only
@@ -3792,7 +3794,7 @@ static int with_agg_find_or_create(with_agg_t **aggs, int *agg_cnt, int *agg_cap
/* Accumulate aggregation values for a binding */
static void with_agg_accumulate(with_agg_t *agg, cbm_return_clause_t *wc, binding_t *b) {
for (int ci = 0; ci < wc->count; ci++) {
if (!wc->items[ci].func) {
if (!is_aggregate_func(wc->items[ci].func)) {
continue;
}
agg->counts[ci]++;
@@ -3887,7 +3889,7 @@ static void execute_with_aggregate(cbm_return_clause_t *wc, binding_t *bindings,
for (int ci = 0; ci < wc->count; ci++) {
char name_buf[CBM_SZ_256];
const char *alias = resolve_item_alias(&wc->items[ci], name_buf, sizeof(name_buf));
if (wc->items[ci].func) {
if (is_aggregate_func(wc->items[ci].func)) {
char vbuf[CBM_SZ_64];
if (wc->items[ci].distinct && strcmp(wc->items[ci].func, "COUNT") == 0) {
snprintf(vbuf, sizeof(vbuf), "%d", aggs[a].distinct_n[ci]); /* #239 */
@@ -4186,7 +4188,7 @@ static void ret_agg_init_group(ret_agg_entry_t *entry, const char *key, int item
/* Accumulate a binding into RETURN aggregation */
static void ret_agg_accumulate(ret_agg_entry_t *entry, cbm_return_clause_t *ret, binding_t *b) {
for (int ci = 0; ci < ret->count; ci++) {
if (!ret->items[ci].func) {
if (!is_aggregate_func(ret->items[ci].func)) {
continue;
}
entry->counts[ci]++;
@@ -4238,7 +4240,7 @@ static void ret_agg_build_key(cbm_return_clause_t *ret, binding_t *b, char *key,
const char **vals, char valbufs[][CBM_SZ_512]) {
int klen = 0;
for (int ci = 0; ci < ret->count; ci++) {
if (ret->items[ci].func) {
if (is_aggregate_func(ret->items[ci].func)) {
vals[ci] = "0";
continue;
}
@@ -4262,7 +4264,7 @@ static void ret_agg_emit_row(cbm_return_clause_t *ret, ret_agg_entry_t *agg, res
const char *row[CBM_SZ_32];
char bufs[CBM_SZ_32][CBM_SZ_64];
for (int ci = 0; ci < ret->count; ci++) {
if (!ret->items[ci].func) {
if (!is_aggregate_func(ret->items[ci].func)) {
row[ci] = agg->group_vals[ci];
continue;
}
tests/test_cypher.c
+44
View File
@@ -2523,6 +2523,28 @@ TEST(cypher_exec_count_star) {
PASS();
}
/* #1111: type(r) grouped with count(*) must return the actual relationship type,
* not the row count. ret_agg_build_key/ret_agg_emit_row classified aggregate vs.
* scalar columns with a bare `item->func` truthy check, so type(r) (a non-aggregate
* function, func != NULL) was misrouted into the aggregate-value branch and
* formatted via format_agg_value's default case, silently substituting the row
* count for the relationship type. */
TEST(cypher_issue1111_return_type_count_group) {
cbm_store_t *s = setup_cypher_store();
cbm_cypher_result_t r = {0};
int rc = cbm_cypher_execute(
s, "MATCH (a)-[r]->(b) RETURN type(r) AS t, count(*) AS n ORDER BY n DESC", "test", 0, &r);
ASSERT_EQ(rc, 0);
ASSERT_EQ(r.row_count, 2);
ASSERT_STR_EQ(r.rows[0][0], "CALLS");
ASSERT_STR_EQ(r.rows[0][1], "3");
ASSERT_STR_EQ(r.rows[1][0], "DEFINES");
ASSERT_STR_EQ(r.rows[1][1], "1");
cbm_cypher_result_free(&r);
cbm_store_close(s);
PASS();
}
TEST(cypher_parse_skip) {
cbm_query_t *q = NULL;
char *err = NULL;
@@ -2707,6 +2729,26 @@ TEST(cypher_exec_with_node_groupvar_prop) {
PASS();
}
/* #1111, WITH variant: the same misrouting in with_agg_build_key/with_agg_accumulate/
* execute_with_aggregate's per-column func check. */
TEST(cypher_issue1111_with_type_count_group) {
cbm_store_t *s = setup_cypher_store();
cbm_cypher_result_t r = {0};
int rc = cbm_cypher_execute(
s,
"MATCH (a)-[r]->(b) WITH type(r) AS t, count(*) AS n RETURN t, n ORDER BY n DESC",
"test", 0, &r);
ASSERT_EQ(rc, 0);
ASSERT_EQ(r.row_count, 2);
ASSERT_STR_EQ(r.rows[0][0], "CALLS");
ASSERT_STR_EQ(r.rows[0][1], "3");
ASSERT_STR_EQ(r.rows[1][0], "DEFINES");
ASSERT_STR_EQ(r.rows[1][1], "1");
cbm_cypher_result_free(&r);
cbm_store_close(s);
PASS();
}
TEST(cypher_exec_with_where) {
cbm_store_t *s = setup_cypher_store();
cbm_cypher_result_t r = {0};
@@ -3402,6 +3444,7 @@ SUITE(cypher) {
RUN_TEST(cypher_exec_max);
RUN_TEST(cypher_exec_collect);
RUN_TEST(cypher_exec_count_star);
RUN_TEST(cypher_issue1111_return_type_count_group);
RUN_TEST(cypher_parse_skip);
RUN_TEST(cypher_parse_sum_avg);
RUN_TEST(cypher_parse_collect);
@@ -3415,6 +3458,7 @@ SUITE(cypher) {
/* Phase 6: WITH clause */
RUN_TEST(cypher_exec_with_rename);
RUN_TEST(cypher_exec_with_count);
RUN_TEST(cypher_issue1111_with_type_count_group);
RUN_TEST(cypher_exec_with_node_groupvar_prop);
RUN_TEST(cypher_exec_with_where);
RUN_TEST(cypher_exec_with_orderby_limit);