From b43bdddcbe46c3b8aab0e9acbdbd81804bcc9dcc Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Thu, 23 Jul 2026 04:21:56 +0000 Subject: [PATCH] fix(cypher): classify aggregation columns by is_aggregate_func, not a 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 #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 --- src/cypher/cypher.c | 20 +++++++++++--------- tests/test_cypher.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 7c394c289..bf8d3c38c 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -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; } diff --git a/tests/test_cypher.c b/tests/test_cypher.c index d25f498dd..3d2aa0c4d 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -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);