diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 7c394c289..a1d7fa34b 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,16 +3771,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; @@ -3792,7 +3798,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 +3893,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 +4192,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 +4244,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 +4268,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..add3bdc3e 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,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}; @@ -3402,6 +3469,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 +3483,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);