Skip to content

fix(cypher): carry the real node through WITH, not a name-only stub (#2208) #2351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DeusData merged 1 commit into from
Sep 30, 2026
Merged

fix(cypher): carry the real node through WITH, not a name-only stub (#2208) #2351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 75 additions & 64 deletions src/cypher/cypher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
Expand Down Expand Up @@ -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.<metric> 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 "";
}

Expand Down Expand Up @@ -3842,16 +3805,39 @@ 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) {
for (int i = 0; i < count - SKIP_ONE; i++) {
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);
Expand Down Expand Up @@ -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.<prop> 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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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++;
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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;
Expand All @@ -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));
Expand Down
Loading
Loading