diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 234f490876..7c7a2f9878 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -2300,7 +2300,6 @@ static const char *node_string_field(const cbm_node_t *n, const char *prop) { /* Get node property by name. * store may be NULL; only needed for virtual degree properties. */ static const char *json_extract_prop(const char *json, const char *key, char *buf, size_t buf_sz); -static void node_fields_free(cbm_node_t *n); /* defined below; used by the stub re-fetch */ static const char *node_prop(const cbm_node_t *n, const char *prop, cbm_store_t *store) { if (!n || !prop) { @@ -2310,8 +2309,6 @@ static const char *node_prop(const cbm_node_t *n, const char *prop, cbm_store_t if (str && str[0]) { return str; } - /* Note: a string field that exists but is empty ("") falls through here so a - * WITH-aggregation node stub (below) can re-fetch it. */ /* Computed and JSON-derived values live in rotating thread-local buffers: * a single row (or an ORDER-BY comparison) reads several of these before any * of them is copied out, so returning one shared static buffer would alias @@ -2349,40 +2346,6 @@ static const char *node_prop(const cbm_node_t *n, const char *prop, cbm_store_t return v; } } - /* WITH aggregation carries a node group var by id + name only (the group key - * is the node name), so every other property is absent on the stub. Detect - * the stub (id set, but the full string fields were never populated) and - * re-fetch the node so RETURN g.file_path / g.label / g. project - * correctly instead of returning blank. The gate is heuristic, not an exact - * stub discriminator: a real bound node with NULL label AND file_path would - * also match, but in that case the worst case is one redundant indexed fetch - * that returns the same value — never a wrong result. */ - if (store && n->id > 0 && !n->file_path && !n->label) { - cbm_node_t full = {0}; - if (cbm_store_find_node_by_id(store, n->id, &full) == CBM_STORE_OK) { - const char *res = NULL; - const char *rv = node_string_field(&full, prop); - if (rv && rv[0]) { - snprintf(out, CBM_SZ_512, "%s", rv); - res = out; - } else if (strcmp(prop, "start_line") == 0) { - snprintf(out, CBM_SZ_512, "%d", full.start_line); - res = out; - } else if (strcmp(prop, "end_line") == 0) { - snprintf(out, CBM_SZ_512, "%d", full.end_line); - res = out; - } else if (full.properties_json && full.properties_json[0] == '{') { - const char *jv = json_extract_prop(full.properties_json, prop, out, CBM_SZ_512); - if (jv && jv[0]) { - res = out; - } - } - node_fields_free(&full); - if (res) { - return res; - } - } - } return ""; } @@ -3842,6 +3805,30 @@ static void distinct_list_add(char ***list, int *count, const char *val) { (*list)[idx] = heap_strdup(val); } +/* Resolve one WITH ORDER BY key on a projected binding. The key is either a + * projected name (`c`, or an unaliased `f.name`) or a property of a carried + * node (`g.start_line` after `WITH f AS g`, #2208): an exact projected name + * wins, otherwise `var.prop` is split and resolved on the carried node. */ +static const char *order_key_value(binding_t *b, const char *key) { + const char *dot = strchr(key, '.'); + if (!dot) { + return binding_get_virtual(b, key, NULL); + } + for (int i = 0; i < b->var_count; i++) { + if (strcmp(b->var_names[i], key) == 0) { + return binding_get_virtual(b, key, NULL); + } + } + char var[CBM_SZ_256]; + size_t vlen = (size_t)(dot - key); + if (vlen >= sizeof(var)) { + return ""; + } + memcpy(var, key, vlen); + var[vlen] = '\0'; + return binding_get_virtual(b, var, dot + SKIP_ONE); +} + /* Sort bindings by the ORDER BY key list (virtual variables) using bubble * sort; later keys break ties, direction is per key (#1334). */ static void sort_bindings(binding_t *vbindings, int count, const cbm_return_clause_t *wc) { @@ -3849,9 +3836,8 @@ static void sort_bindings(binding_t *vbindings, int count, const cbm_return_clau for (int j = 0; j < count - i - SKIP_ONE; j++) { int cmp = 0; for (int k = 0; k < wc->order_key_count && cmp == 0; k++) { - const char *va = binding_get_virtual(&vbindings[j], wc->order_keys[k], NULL); - const char *vb2 = - binding_get_virtual(&vbindings[j + SKIP_ONE], wc->order_keys[k], NULL); + const char *va = order_key_value(&vbindings[j], wc->order_keys[k]); + const char *vb2 = order_key_value(&vbindings[j + SKIP_ONE], wc->order_keys[k]); char *ea = NULL; char *eb = NULL; double da = strtod(va, &ea); @@ -3925,9 +3911,28 @@ typedef struct { double *mins, *maxs; char ***distinct_lists; /* per-item set of seen values for COUNT(DISTINCT) */ int *distinct_n; /* per-item distinct count (#239) */ - int64_t *group_node_ids; /* per-item node id when the group var is a node (0 = not) */ + cbm_node_t *group_nodes; /* per-item carried node (deep copy; id 0 = not a node) */ } with_agg_t; +/* A WITH item that is a bare, bound node variable (`WITH f`, `WITH f AS g`) + * carries the node itself, not a scalar: return that node, else NULL. Property + * items, functions (labels/id/keys/...) and unbound OPTIONAL vars (id 0) stay + * scalar. The carried node keeps every field, so a later g. resolves + * exactly as it would without the WITH (#2208). */ +static const cbm_node_t *with_item_carried_node(const cbm_return_item_t *item, binding_t *b) { + if (item->func || item->property || !item->variable) { + return NULL; + } + const cbm_node_t *n = binding_get(b, item->variable); + return (n && n->id > 0) ? n : NULL; +} + +/* Name a carried node is bound under: the AST-owned alias or variable, which + * outlives every binding of the query (binding_set does not copy the name). */ +static const char *with_item_node_alias(const cbm_return_item_t *item) { + return item->alias ? item->alias : item->variable; +} + /* Build a group key from non-aggregate WITH items */ static int with_agg_build_key(cbm_return_clause_t *wc, binding_t *b, char *key, size_t key_sz) { int kl = 0; @@ -3936,7 +3941,12 @@ static int with_agg_build_key(cbm_return_clause_t *wc, binding_t *b, char *key, continue; } char vbuf[CBM_SZ_512]; - const char *v = project_item(b, &wc->items[ci], vbuf, sizeof(vbuf)); + const cbm_node_t *gn = with_item_carried_node(&wc->items[ci], b); + if (gn) { + /* Group a node by identity, never by its (non-unique) name. */ + snprintf(vbuf, sizeof(vbuf), "#%lld", (long long)gn->id); + } + const char *v = gn ? vbuf : 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; @@ -3966,7 +3976,7 @@ static int with_agg_find_or_create(with_agg_t **aggs, int *agg_cnt, int *agg_cap (*aggs)[found].maxs = calloc(wc->count, sizeof(double)); (*aggs)[found].distinct_lists = calloc(wc->count, sizeof(char **)); (*aggs)[found].distinct_n = calloc(wc->count, sizeof(int)); - (*aggs)[found].group_node_ids = calloc(wc->count, sizeof(int64_t)); + (*aggs)[found].group_nodes = calloc(wc->count, sizeof(cbm_node_t)); for (int ci = 0; ci < wc->count; ci++) { (*aggs)[found].mins[ci] = CYP_DBL_MAX; (*aggs)[found].maxs[ci] = -CYP_DBL_MAX; @@ -3979,18 +3989,9 @@ static int with_agg_find_or_create(with_agg_t **aggs, int *agg_cnt, int *agg_cap 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). 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; - } + const cbm_node_t *gn = with_item_carried_node(&wc->items[ci], b); + if (gn) { + node_deep_copy(&(*aggs)[found].group_nodes[ci], gn); } } return found; @@ -4033,11 +4034,14 @@ static void with_agg_format(const char *func, with_agg_t *agg, int ci, char *buf } } -/* Add a virtual variable binding for one WITH item */ +/* Add a scalar virtual variable binding for one WITH item. The value lives in + * .name; the owned alias string (var_names points at it) lives in .project, + * which no property accessor exposes. It used to live in .qualified_name, so + * alias.qualified_name / keys(alias) returned the variable name as data (#2208). */ static void with_add_vbinding_var(binding_t *vb, const char *alias, const char *val) { - cbm_node_t vn = {.name = heap_strdup(val), .qualified_name = heap_strdup(alias)}; + cbm_node_t vn = {.name = heap_strdup(val), .project = heap_strdup(alias)}; if (vb->var_count < CYP_BUF_16) { - vb->var_names[vb->var_count] = vn.qualified_name; + vb->var_names[vb->var_count] = vn.project; vb->var_nodes[vb->var_count] = vn; vb->var_count++; } @@ -4062,7 +4066,12 @@ static void with_agg_free(with_agg_t *aggs, int agg_cnt, int item_count) { free(aggs[a].maxs); free(aggs[a].distinct_lists); free(aggs[a].distinct_n); - free(aggs[a].group_node_ids); + if (aggs[a].group_nodes) { + for (int ci = 0; ci < item_count; ci++) { + node_fields_free(&aggs[a].group_nodes[ci]); + } + } + free(aggs[a].group_nodes); } free(aggs); } @@ -4102,13 +4111,10 @@ static void execute_with_aggregate(cbm_return_clause_t *wc, binding_t *bindings, with_agg_format(wc->items[ci].func, &aggs[a], ci, vbuf, sizeof(vbuf)); } with_add_vbinding_var(&vb, alias, vbuf); + } else if (aggs[a].group_nodes[ci].id > 0) { + binding_set(&vb, with_item_node_alias(&wc->items[ci]), &aggs[a].group_nodes[ci]); } else { with_add_vbinding_var(&vb, alias, aggs[a].group_vals[ci]); - /* Tag the carried virtual var with the node id (when the group - * var is a node) so node_prop can re-fetch its full properties. */ - if (aggs[a].group_node_ids[ci] > 0 && vb.var_count > 0) { - vb.var_nodes[vb.var_count - 1].id = aggs[a].group_node_ids[ci]; - } } } (*vbindings)[(*vcount)++] = vb; @@ -4125,6 +4131,11 @@ static void execute_with_simple(cbm_return_clause_t *wc, binding_t *bindings, in 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)); + const cbm_node_t *carried = with_item_carried_node(&wc->items[ci], &bindings[bi]); + if (carried) { + binding_set(&vb, with_item_node_alias(&wc->items[ci]), carried); + continue; + } char func_buf[CBM_SZ_512]; const char *val = project_item(&bindings[bi], &wc->items[ci], func_buf, sizeof(func_buf)); diff --git a/tests/test_cypher.c b/tests/test_cypher.c index 8bdee76c9a..7c251bd9b0 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -3937,6 +3937,202 @@ TEST(cypher_exec_with_rename) { PASS(); } +/* #2208: a node carried IN SCOPE through WITH must project every property + * exactly as the same node does without the WITH. The carried var used to be a + * name-only stub whose qualified_name slot held the variable name and whose + * start_line was 0, so f.qualified_name returned "f" and f.start_line "0". */ +static int64_t issue2208_id_of(cbm_store_t *s, const char *qn) { + cbm_node_t n = {0}; + int64_t id = 0; + if (cbm_store_find_node_by_qn(s, "test", qn, &n) == CBM_STORE_OK) { + id = n.id; + cbm_node_free_fields(&n); + } + return id; +} + +static cbm_store_t *setup_issue2208_store(void) { + cbm_store_t *s = setup_cypher_store(); + cbm_node_t twins[] = { + {.project = "test", + .label = "Function", + .name = "ProjectionTwin", + .qualified_name = "test.ProjectionA", + .file_path = "projection_a.go", + .start_line = 30, + .end_line = 40, + .properties_json = "{\"marker\":\"alpha\"}"}, + {.project = "test", + .label = "Function", + .name = "ProjectionTwin", + .qualified_name = "test.ProjectionB", + .file_path = "projection_b.go", + .start_line = 10, + .end_line = 20, + .properties_json = "{\"marker\":\"beta\"}"}, + {.project = "test", + .label = "Function", + .name = "ProjectionTwin", + .qualified_name = "test.ProjectionC", + .file_path = "projection_c.go", + .start_line = 20, + .end_line = 30, + .properties_json = "{\"marker\":\"gamma\"}"}, + }; + int64_t ids[3]; + for (int i = 0; i < 3; i++) { + ids[i] = cbm_store_upsert_node(s, &twins[i]); + } + /* Callers: A has 2 (HandleOrder, ValidateOrder), B has 1 (HandleOrder), C none. */ + int64_t handle = issue2208_id_of(s, "test.HandleOrder"); + int64_t validate = issue2208_id_of(s, "test.ValidateOrder"); + cbm_edge_t e1 = {.project = "test", .source_id = handle, .target_id = ids[0], .type = "CALLS"}; + cbm_edge_t e2 = { + .project = "test", .source_id = validate, .target_id = ids[0], .type = "CALLS"}; + cbm_edge_t e3 = {.project = "test", .source_id = handle, .target_id = ids[1], .type = "CALLS"}; + cbm_store_insert_edge(s, &e1); + cbm_store_insert_edge(s, &e2); + cbm_store_insert_edge(s, &e3); + return s; +} + +/* The reporter's exact shape: aggregate WITH carrying the node beside count(). */ +TEST(cypher_issue2208_with_agg_carried_node_props) { + cbm_store_t *s = setup_issue2208_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, + "MATCH (f:Function {qualified_name: 'test.ProjectionA'}) " + "OPTIONAL MATCH (caller)-[r:CALLS]->(f) " + "WITH f, count(r) AS refs " + "RETURN f.qualified_name, f.file_path, f.start_line, " + "f.end_line, f.label, f.marker, f.name, refs", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], "test.ProjectionA"); + ASSERT_STR_EQ(r.rows[0][1], "projection_a.go"); + ASSERT_STR_EQ(r.rows[0][2], "30"); + ASSERT_STR_EQ(r.rows[0][3], "40"); + ASSERT_STR_EQ(r.rows[0][4], "Function"); + ASSERT_STR_EQ(r.rows[0][5], "alpha"); + ASSERT_STR_EQ(r.rows[0][6], "ProjectionTwin"); + ASSERT_STR_EQ(r.rows[0][7], "2"); + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +/* Aggregate WITH groups by node IDENTITY, not display name: three same-named + * functions stay three rows, each with its own properties and caller count + * (they used to collapse into one row with one node's props and summed refs). */ +TEST(cypher_issue2208_with_agg_groups_by_node_identity) { + cbm_store_t *s = setup_issue2208_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, + "MATCH (f:Function {name: 'ProjectionTwin'}) " + "OPTIONAL MATCH (caller)-[r:CALLS]->(f) " + "WITH f, count(r) AS refs " + "RETURN f.qualified_name, f.start_line, refs " + "ORDER BY f.qualified_name", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 3); + ASSERT_STR_EQ(r.rows[0][0], "test.ProjectionA"); + ASSERT_STR_EQ(r.rows[0][1], "30"); + ASSERT_STR_EQ(r.rows[0][2], "2"); + ASSERT_STR_EQ(r.rows[1][0], "test.ProjectionB"); + ASSERT_STR_EQ(r.rows[1][1], "10"); + ASSERT_STR_EQ(r.rows[1][2], "1"); + ASSERT_STR_EQ(r.rows[2][0], "test.ProjectionC"); + ASSERT_STR_EQ(r.rows[2][1], "20"); + /* rows[2][2] (count(r) for the caller-less C) is not asserted here: count() + * of an unbound OPTIONAL var is a separate, pre-existing defect. */ + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +/* Simple (non-aggregate) WITH: a bare node stays a node (renamed or not); + * scalar items stay scalars; OPTIONAL null stays null; post-WITH WHERE and + * WITH ... ORDER BY see the real node properties. */ +TEST(cypher_issue2208_with_simple_carried_node_props) { + cbm_store_t *s = setup_issue2208_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, + "MATCH (f:Function) WHERE f.qualified_name = 'test.ProjectionA' " + "WITH f " + "RETURN f.name, f.qualified_name, f.file_path, f.label, " + "f.start_line, f.marker", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], "ProjectionTwin"); + ASSERT_STR_EQ(r.rows[0][1], "test.ProjectionA"); + ASSERT_STR_EQ(r.rows[0][2], "projection_a.go"); + ASSERT_STR_EQ(r.rows[0][3], "Function"); + ASSERT_STR_EQ(r.rows[0][4], "30"); + ASSERT_STR_EQ(r.rows[0][5], "alpha"); + cbm_cypher_result_free(&r); + + memset(&r, 0, sizeof(r)); + rc = cbm_cypher_execute(s, + "MATCH (f:Function) WHERE f.qualified_name = 'test.ProjectionA' " + "WITH f AS g, f.name AS n, labels(f) AS ls " + "RETURN g.qualified_name, g.file_path, n, ls, n.file_path, " + "ls.file_path", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], "test.ProjectionA"); + ASSERT_STR_EQ(r.rows[0][1], "projection_a.go"); + ASSERT_STR_EQ(r.rows[0][2], "ProjectionTwin"); + ASSERT_STR_EQ(r.rows[0][3], "[\"Function\"]"); + ASSERT_STR_EQ(r.rows[0][4], ""); + ASSERT_STR_EQ(r.rows[0][5], ""); + cbm_cypher_result_free(&r); + + /* OPTIONAL null carried through WITH stays null (no fabricated node). */ + memset(&r, 0, sizeof(r)); + rc = cbm_cypher_execute(s, + "MATCH (f:Function) WHERE f.name = 'LogError' " + "OPTIONAL MATCH (f)-[:CALLS]->(g:Function) " + "WITH g AS projected " + "RETURN projected, projected.file_path, projected.qualified_name", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], ""); + ASSERT_STR_EQ(r.rows[0][1], ""); + ASSERT_STR_EQ(r.rows[0][2], ""); + cbm_cypher_result_free(&r); + + memset(&r, 0, sizeof(r)); + rc = cbm_cypher_execute(s, + "MATCH (f:Function) WITH f AS g " + "WHERE g.file_path = 'projection_b.go' " + "RETURN g.qualified_name", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], "test.ProjectionB"); + cbm_cypher_result_free(&r); + + /* A(30), B(10), C(20): ascending then SKIP 1 LIMIT 1 selects C. */ + memset(&r, 0, sizeof(r)); + rc = cbm_cypher_execute(s, + "MATCH (f:Function) WHERE f.name = 'ProjectionTwin' " + "WITH f AS g ORDER BY g.start_line ASC SKIP 1 LIMIT 1 " + "RETURN g.qualified_name, g.start_line", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], "test.ProjectionC"); + ASSERT_STR_EQ(r.rows[0][1], "20"); + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + TEST(cypher_exec_with_count) { cbm_store_t *s = setup_cypher_store(); cbm_cypher_result_t r = {0}; @@ -4822,6 +5018,9 @@ SUITE(cypher) { RUN_TEST(cypher_parse_case); /* Phase 6: WITH clause */ RUN_TEST(cypher_exec_with_rename); + RUN_TEST(cypher_issue2208_with_agg_carried_node_props); + RUN_TEST(cypher_issue2208_with_agg_groups_by_node_identity); + RUN_TEST(cypher_issue2208_with_simple_carried_node_props); 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);