Skip to content

Commit 2bf474a

Browse files
fix(cypher): preserve node properties through WITH aggregation
A node group variable carried through a WITH aggregation (e.g. `WITH g, count(*) AS c RETURN g.file_path`) returned blank for every property except its name: the carried virtual binding held only the group key (the node's name) and lacked a store handle, so node_prop() could neither read other fields nor compute degrees. Fix: capture the node id of a bare node group-var in with_agg_find_or_create and tag the carried virtual binding with it; in node_prop(), when such a stub (id set, string fields unpopulated) is asked for a missing property, re-fetch the full node via cbm_store_find_node_by_id and project it. Also propagate the store onto virtual bindings so node_prop can re-fetch and compute degrees. The stub gate is heuristic but never yields a wrong value — worst case is one redundant indexed lookup. Adds regression test cypher_exec_with_node_groupvar_prop. Signed-off-by: Kris Kersey <kris@kerseyfabrications.com>
1 parent e599df1 commit 2bf474a

2 files changed

Lines changed: 83 additions & 3 deletions

File tree

src/cypher/cypher.c

Lines changed: 61 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2061,15 +2061,18 @@ static const char *node_string_field(const cbm_node_t *n, const char *prop) {
20612061
/* Get node property by name.
20622062
* store may be NULL; only needed for virtual degree properties. */
20632063
static const char *json_extract_prop(const char *json, const char *key, char *buf, size_t buf_sz);
2064+
static void node_fields_free(cbm_node_t *n); /* defined below; used by the stub re-fetch */
20642065

20652066
static const char *node_prop(const cbm_node_t *n, const char *prop, cbm_store_t *store) {
20662067
if (!n || !prop) {
20672068
return "";
20682069
}
20692070
const char *str = node_string_field(n, prop);
2070-
if (str) {
2071+
if (str && str[0]) {
20712072
return str;
20722073
}
2074+
/* Note: a string field that exists but is empty ("") falls through here so a
2075+
* WITH-aggregation node stub (below) can re-fetch it. */
20732076
/* Computed and JSON-derived values live in rotating thread-local buffers:
20742077
* a single row (or an ORDER-BY comparison) reads several of these before any
20752078
* of them is copied out, so returning one shared static buffer would alias
@@ -2107,6 +2110,40 @@ static const char *node_prop(const cbm_node_t *n, const char *prop, cbm_store_t
21072110
return v;
21082111
}
21092112
}
2113+
/* WITH aggregation carries a node group var by id + name only (the group key
2114+
* is the node name), so every other property is absent on the stub. Detect
2115+
* the stub (id set, but the full string fields were never populated) and
2116+
* re-fetch the node so RETURN g.file_path / g.label / g.<metric> project
2117+
* correctly instead of returning blank. The gate is heuristic, not an exact
2118+
* stub discriminator: a real bound node with NULL label AND file_path would
2119+
* also match, but in that case the worst case is one redundant indexed fetch
2120+
* that returns the same value — never a wrong result. */
2121+
if (store && n->id > 0 && !n->file_path && !n->label) {
2122+
cbm_node_t full = {0};
2123+
if (cbm_store_find_node_by_id(store, n->id, &full) == CBM_STORE_OK) {
2124+
const char *res = NULL;
2125+
const char *rv = node_string_field(&full, prop);
2126+
if (rv && rv[0]) {
2127+
snprintf(out, CBM_SZ_512, "%s", rv);
2128+
res = out;
2129+
} else if (strcmp(prop, "start_line") == 0) {
2130+
snprintf(out, CBM_SZ_512, "%d", full.start_line);
2131+
res = out;
2132+
} else if (strcmp(prop, "end_line") == 0) {
2133+
snprintf(out, CBM_SZ_512, "%d", full.end_line);
2134+
res = out;
2135+
} else if (full.properties_json && full.properties_json[0] == '{') {
2136+
const char *jv = json_extract_prop(full.properties_json, prop, out, CBM_SZ_512);
2137+
if (jv && jv[0]) {
2138+
res = out;
2139+
}
2140+
}
2141+
node_fields_free(&full);
2142+
if (res) {
2143+
return res;
2144+
}
2145+
}
2146+
}
21102147
return "";
21112148
}
21122149

@@ -3406,8 +3443,9 @@ typedef struct {
34063443
double *sums;
34073444
int *counts;
34083445
double *mins, *maxs;
3409-
char ***distinct_lists; /* per-item set of seen values for COUNT(DISTINCT) */
3410-
int *distinct_n; /* per-item distinct count (#239) */
3446+
char ***distinct_lists; /* per-item set of seen values for COUNT(DISTINCT) */
3447+
int *distinct_n; /* per-item distinct count (#239) */
3448+
int64_t *group_node_ids; /* per-item node id when the group var is a node (0 = not) */
34113449
} with_agg_t;
34123450

34133451
/* Build a group key from non-aggregate WITH items */
@@ -3447,6 +3485,7 @@ static int with_agg_find_or_create(with_agg_t **aggs, int *agg_cnt, int *agg_cap
34473485
(*aggs)[found].maxs = calloc(wc->count, sizeof(double));
34483486
(*aggs)[found].distinct_lists = calloc(wc->count, sizeof(char **));
34493487
(*aggs)[found].distinct_n = calloc(wc->count, sizeof(int));
3488+
(*aggs)[found].group_node_ids = calloc(wc->count, sizeof(int64_t));
34503489
for (int ci = 0; ci < wc->count; ci++) {
34513490
(*aggs)[found].mins[ci] = CYP_DBL_MAX;
34523491
(*aggs)[found].maxs[ci] = -CYP_DBL_MAX;
@@ -3458,6 +3497,15 @@ static int with_agg_find_or_create(with_agg_t **aggs, int *agg_cnt, int *agg_cap
34583497
}
34593498
const char *v = binding_get_virtual(b, wc->items[ci].variable, wc->items[ci].property);
34603499
(*aggs)[found].group_vals[ci] = heap_strdup(v);
3500+
/* If this group item is a bare node variable, remember its id so the
3501+
* carried virtual var can re-fetch any property (group_vals holds only
3502+
* the name). */
3503+
if (!wc->items[ci].property && wc->items[ci].variable) {
3504+
cbm_node_t *gn = binding_get(b, wc->items[ci].variable);
3505+
if (gn) {
3506+
(*aggs)[found].group_node_ids[ci] = gn->id;
3507+
}
3508+
}
34613509
}
34623510
return found;
34633511
}
@@ -3528,6 +3576,7 @@ static void with_agg_free(with_agg_t *aggs, int agg_cnt, int item_count) {
35283576
free(aggs[a].maxs);
35293577
free(aggs[a].distinct_lists);
35303578
free(aggs[a].distinct_n);
3579+
free(aggs[a].group_node_ids);
35313580
}
35323581
free(aggs);
35333582
}
@@ -3553,6 +3602,9 @@ static void execute_with_aggregate(cbm_return_clause_t *wc, binding_t *bindings,
35533602
}
35543603
for (int a = 0; a < agg_cnt; a++) {
35553604
binding_t vb = {0};
3605+
/* Carry the store so node_prop can re-fetch a carried node's properties
3606+
* (and compute in_degree/out_degree) on the projected virtual binding. */
3607+
vb.store = (bind_count > 0) ? bindings[0].store : NULL;
35563608
for (int ci = 0; ci < wc->count; ci++) {
35573609
char name_buf[CBM_SZ_256];
35583610
const char *alias = resolve_item_alias(&wc->items[ci], name_buf, sizeof(name_buf));
@@ -3566,6 +3618,11 @@ static void execute_with_aggregate(cbm_return_clause_t *wc, binding_t *bindings,
35663618
with_add_vbinding_var(&vb, alias, vbuf);
35673619
} else {
35683620
with_add_vbinding_var(&vb, alias, aggs[a].group_vals[ci]);
3621+
/* Tag the carried virtual var with the node id (when the group
3622+
* var is a node) so node_prop can re-fetch its full properties. */
3623+
if (aggs[a].group_node_ids[ci] > 0 && vb.var_count > 0) {
3624+
vb.var_nodes[vb.var_count - 1].id = aggs[a].group_node_ids[ci];
3625+
}
35693626
}
35703627
}
35713628
(*vbindings)[(*vcount)++] = vb;
@@ -3578,6 +3635,7 @@ static void execute_with_simple(cbm_return_clause_t *wc, binding_t *bindings, in
35783635
binding_t *vbindings, int *vcount) {
35793636
for (int bi = 0; bi < bind_count; bi++) {
35803637
binding_t vb = {0};
3638+
vb.store = bindings[bi].store; /* so node_prop can re-fetch / compute on the projection */
35813639
for (int ci = 0; ci < wc->count; ci++) {
35823640
char name_buf[CBM_SZ_256];
35833641
const char *alias = resolve_item_alias(&wc->items[ci], name_buf, sizeof(name_buf));

tests/test_cypher.c

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2183,6 +2183,27 @@ TEST(cypher_exec_with_count) {
21832183
PASS();
21842184
}
21852185

2186+
/* Regression: a bare node group-var carried through WITH aggregation must project
2187+
* its real properties (not blank). Pre-fix, the carried var held only the node
2188+
* name, so RETURN g.file_path returned "". */
2189+
TEST(cypher_exec_with_node_groupvar_prop) {
2190+
cbm_store_t *s = setup_cypher_store();
2191+
cbm_cypher_result_t r = {0};
2192+
int rc = cbm_cypher_execute(s,
2193+
"MATCH (f:Function)-[:CALLS]->(g:Function) "
2194+
"WHERE g.name = \"ValidateOrder\" "
2195+
"WITH g, COUNT(*) AS c "
2196+
"RETURN g.file_path, g.name, c",
2197+
"test", 0, &r);
2198+
ASSERT_EQ(rc, 0);
2199+
ASSERT_EQ(r.row_count, 1);
2200+
ASSERT_STR_EQ(r.rows[0][0], "validate.go"); /* was "" before the fix */
2201+
ASSERT_STR_EQ(r.rows[0][1], "ValidateOrder");
2202+
cbm_cypher_result_free(&r);
2203+
cbm_store_close(s);
2204+
PASS();
2205+
}
2206+
21862207
TEST(cypher_exec_with_where) {
21872208
cbm_store_t *s = setup_cypher_store();
21882209
cbm_cypher_result_t r = {0};
@@ -2642,6 +2663,7 @@ SUITE(cypher) {
26422663
/* Phase 6: WITH clause */
26432664
RUN_TEST(cypher_exec_with_rename);
26442665
RUN_TEST(cypher_exec_with_count);
2666+
RUN_TEST(cypher_exec_with_node_groupvar_prop);
26452667
RUN_TEST(cypher_exec_with_where);
26462668
RUN_TEST(cypher_exec_with_orderby_limit);
26472669
RUN_TEST(cypher_parse_with);

0 commit comments

Comments
 (0)