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

Merge pull request #1221 from AmirF194/fix/1111-aggregate-scalar-func-misclassification

Browse Source 
fix(cypher): classify aggregation columns by is_aggregate_func, not a bare func check
This commit is contained in:
Martin Vogel
2026-07-31 09:59:09 +02:00
committed by GitHub
parent 7814e1bf9c 2f522b40cc
commit 95958222c8
2 changed files with 87 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/cypher/cypher.c
+17 -11
View File
@@ -3742,10 +3742,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;
@@ -3781,16 +3782,21 @@ 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
* the name). */
if (!wc->items[ci].property && wc->items[ci].variable) {
* the name). Excludes entity-introspection funcs (labels/id/keys/
* properties): those project a scalar off the node (via project_item
* above), not the node itself, so the carried id must not be set or a
* later alias.property re-fetches the source node's real properties
* instead of returning empty for the non-node alias. */
if (!wc->items[ci].func && !wc->items[ci].property && wc->items[ci].variable) {
cbm_node_t *gn = binding_get(b, wc->items[ci].variable);
if (gn) {
(*aggs)[found].group_node_ids[ci] = gn->id;
@@ -3803,7 +3809,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]++;
@@ -3898,7 +3904,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 */
@@ -4197,7 +4203,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]++;
@@ -4249,7 +4255,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;
}
@@ -4273,7 +4279,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
+70
View File
@@ -2652,6 +2652,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;
@@ -2836,6 +2858,51 @@ 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();
}
/* #1111 follow-up (review from DeusData on #1221): with_agg_find_or_create's
* bare-node-carry check only tested `!property && variable`, so an entity-
* introspection alias like `labels(f) AS l` (variable set, property NULL, func
* set) was ALSO tagged with the source node's id. A later `l.file_path` then
* hit node_prop's stub re-fetch heuristic (id set, file_path/label both NULL on
* the virtual stub) and silently returned HandleOrder's real file_path instead
* of "" for the non-node alias `l`. */
TEST(cypher_issue1111_with_scalar_func_alias_no_node_leak) {
cbm_store_t *s = setup_cypher_store();
cbm_cypher_result_t r = {0};
int rc = cbm_cypher_execute(s,
"MATCH (f:Function) WHERE f.name = \"HandleOrder\" "
"WITH labels(f) AS l, COUNT(*) AS c "
"RETURN l, l.file_path, c",
"test", 0, &r);
ASSERT_EQ(rc, 0);
ASSERT_EQ(r.row_count, 1);
ASSERT_STR_EQ(r.rows[0][0], "[\"Function\"]");
ASSERT_STR_EQ(r.rows[0][1], ""); /* was "handler.go" before the fix */
ASSERT_STR_EQ(r.rows[0][2], "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};
@@ -3533,6 +3600,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);
@@ -3546,6 +3614,8 @@ 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_issue1111_with_scalar_func_alias_no_node_leak);
RUN_TEST(cypher_exec_with_node_groupvar_prop);
RUN_TEST(cypher_exec_with_where);
RUN_TEST(cypher_exec_with_orderby_limit);