Skip to content

fix(cypher): expansion materializes every matched row — the cap falsified aggregates (#1196) #1698

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
Aug 18, 2026
+164 −85
Merged

fix(cypher): expansion materializes every matched row — the cap falsified aggregates (#1196) #1698

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
143 changes: 77 additions & 66 deletions src/cypher/cypher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ enum {
CYP_MAX_VARS = 16, /* max Cypher variables in a query */
CYP_MAX_EDGE_VARS = 8, /* max edge variables */
CYP_GROWTH_10 = 10, /* binding growth factor */
CYP_GROWTH_2 = 2, /* geometric buffer growth (#1196) */
CYP_CHAR_IDX1 = 1, /* second character index (e.g. op[1]) */
CYP_EBUF_MASK = 7,
CYP_NODE_COLS = 4, /* columns per node var: name, qn, label, file */
Expand Down Expand Up @@ -3101,10 +3102,33 @@ static void scan_pattern_nodes(cbm_store_t *store, const char *project, cbm_node

/* Process edges: look up target node, filter by label/props, add binding.
* `inbound` controls which end of the edge is the target id. */
/* #1196 (second mechanism): a hop's output buffer must hold EVERY matched
* row. max_rows is an OUTPUT-row limit — projection already enforces it — and
* WHERE/aggregation run after expansion, so any cap here silently falsifies
* results: field-measured, count() reported 9,360 of 13,691 DEFINES because
* the old bind_cap*10 ceiling dropped edges before aggregation ever saw them,
* and a labeled source did not save you. The buffer now grows geometrically;
* only allocation failure stops materialisation (the #601 deadline still
* bounds time), and match_count stays truthful either way (#627 contract). */
static bool binding_out_append(binding_t **rows, int *count, int *cap, binding_t *nb) {
if (*count == *cap) {
int next = *cap > 0 ? *cap * CYP_GROWTH_2 : CYP_GROWTH_10;
binding_t *grown = realloc(*rows, (size_t)next * sizeof(binding_t));
if (!grown) {
binding_free(nb);
return false;
}
*rows = grown;
*cap = next;
}
(*rows)[(*count)++] = *nb;
return true;
}

static void process_edges(cbm_store_t *store, cbm_edge_t *edges, int edge_count, bool inbound,
const cbm_node_pattern_t *target_node, binding_t *b, const char *to_var,
const char *rel_var, binding_t *new_bindings, int *new_count, int max_new,
int *match_count) {
const char *rel_var, binding_t **new_bindings, int *new_count,
int *new_cap, int *match_count) {
/* When the terminal node variable is ALREADY bound (e.g. the second pattern
* `(c)-[:CALLS]->(f)` where `f` came from an earlier MATCH), we must FILTER
* to edges that actually reach the bound node — not overwrite the caller's
Expand Down Expand Up @@ -3137,16 +3161,14 @@ static void process_edges(cbm_store_t *store, cbm_edge_t *edges, int edge_count,
node_fields_free(&found);
continue;
}
(*match_count)++; /* a real neighbour exists, budget or not */
if (*new_count < max_new) {
binding_t nb = {0};
binding_copy(&nb, b);
binding_set(&nb, to_var, &found);
if (rel_var) {
binding_set_edge(&nb, rel_var, &edges[ei]);
}
new_bindings[(*new_count)++] = nb;
(*match_count)++; /* a real neighbour exists, OOM or not */
binding_t nb = {0};
binding_copy(&nb, b);
binding_set(&nb, to_var, &found);
if (rel_var) {
binding_set_edge(&nb, rel_var, &edges[ei]);
}
(void)binding_out_append(new_bindings, new_count, new_cap, &nb);
node_fields_free(&found);
}
}
Expand All @@ -3161,8 +3183,8 @@ static _Thread_local int g_cypher_depth_clamped = 0;

static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
cbm_node_pattern_t *target_node, binding_t *b, cbm_node_t *src,
const char *to_var, binding_t *new_bindings, int *new_count,
int max_new, int *match_count) {
const char *to_var, binding_t **new_bindings, int *new_count,
int *new_cap, int *match_count) {
/* Clamp BOTH the explicit (`*1..N`) and unbounded (`*`, `*..m`) forms to the
* engine ceiling: an explicit N above the cap was previously honoured
* verbatim, driving cbm_store_bfs to an unbounded hop count (#887). WARN on
Expand Down Expand Up @@ -3201,21 +3223,19 @@ static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
continue;
}
(*match_count)++;
if (*new_count < max_new) {
binding_t nb = {0};
binding_copy(&nb, b);
binding_set(&nb, to_var, &hop->node);
new_bindings[(*new_count)++] = nb;
}
binding_t nb = {0};
binding_copy(&nb, b);
binding_set(&nb, to_var, &hop->node);
(void)binding_out_append(new_bindings, new_count, new_cap, &nb);
}
cbm_store_traverse_free(&tr);
}

/* Expand fixed-length (1-hop) relationship edges */
static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
cbm_node_pattern_t *target_node, binding_t *b, cbm_node_t *src,
const char *to_var, binding_t *new_bindings, int *new_count,
int max_new, int *match_count) {
const char *to_var, binding_t **new_bindings, int *new_count,
int *new_cap, int *match_count) {
bool is_inbound = rel->direction && strcmp(rel->direction, "inbound") == 0;
bool is_any = rel->direction && strcmp(rel->direction, "any") == 0;
const char *rel_var = rel->variable;
Expand All @@ -3232,7 +3252,7 @@ static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
&edge_count);
}
process_edges(store, edges, edge_count, is_inbound, target_node, b, to_var, rel_var,
new_bindings, new_count, max_new, match_count);
new_bindings, new_count, new_cap, match_count);
cbm_store_free_edges(edges, edge_count);
}
if (is_any) {
Expand All @@ -3242,7 +3262,7 @@ static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
cbm_store_find_edges_by_target_type(store, src->id, rel->types[ti], &edges,
&edge_count);
process_edges(store, edges, edge_count, true, target_node, b, to_var, rel_var,
new_bindings, new_count, max_new, match_count);
new_bindings, new_count, new_cap, match_count);
cbm_store_free_edges(edges, edge_count);
}
}
Expand All @@ -3255,21 +3275,21 @@ static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
cbm_store_find_edges_by_source(store, src->id, &edges, &edge_count);
}
process_edges(store, edges, edge_count, is_inbound, target_node, b, to_var, rel_var,
new_bindings, new_count, max_new, match_count);
new_bindings, new_count, new_cap, match_count);
cbm_store_free_edges(edges, edge_count);
if (is_any) {
edges = NULL;
edge_count = 0;
cbm_store_find_edges_by_target(store, src->id, &edges, &edge_count);
process_edges(store, edges, edge_count, true, target_node, b, to_var, rel_var,
new_bindings, new_count, max_new, match_count);
new_bindings, new_count, new_cap, match_count);
cbm_store_free_edges(edges, edge_count);
}
}
}

static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_t **bindings,
int *bind_count, const int *bind_cap, const char **var_name,
int *bind_count, int *bind_cap, const char **var_name,
bool is_optional) {
for (int ri = 0; ri < pat->rel_count; ri++) {
/* #601: stop expanding further hops once the wall-clock budget is spent
Expand All @@ -3283,16 +3303,14 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_

bool is_variable_length = (rel->min_hops != SKIP_ONE || rel->max_hops != SKIP_ONE);

/* Size this hop's output for BOTH writers without dropping any row: the
* expansion helpers emit at most max_new = bind_cap*10 rows (they stop at
* max_new), and the OPTIONAL fallback emits at most one row per source
* (<= *bind_count). A source either matches (feeds the expansion) or takes
* the fallback, never both, so the two counts are additive and bounded by
* max_new + *bind_count. Computed in size_t so the product cannot overflow.
* The previous "+ 1" sizing fit only a SINGLE fallback row after a
* saturated expansion; a second one ran off the end (heap OOB, CWE-787). */
size_t alloc_n = (size_t)*bind_cap * (size_t)CYP_GROWTH_10 + (size_t)*bind_count;
binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t));
/* #1196: the hop's output buffer GROWS to hold every matched row —
* the old bind_cap*10 ceiling silently dropped edges before WHERE and
* aggregation, falsifying counts. binding_out_append handles growth
* and the OPTIONAL fallback shares it, so no writer can run off the
* end and no row class is dropped (the old fixed sizing had exactly
* those two failure modes, CWE-787 and the OPTIONAL data loss). */
int new_cap = *bind_count > 0 ? *bind_count : CYP_GROWTH_10;
binding_t *new_bindings = malloc((size_t)new_cap * sizeof(binding_t));
if (!new_bindings) {
return; /* OOM: leave existing bindings untouched rather than corrupt */
}
Expand All @@ -3310,24 +3328,22 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_

int match_count = 0;

int max_new = *bind_cap * CYP_GROWTH_10;
if (is_variable_length) {
expand_var_length(store, rel, target_node, b, src, to_var, new_bindings, &new_count,
max_new, &match_count);
expand_var_length(store, rel, target_node, b, src, to_var, &new_bindings,
&new_count, &new_cap, &match_count);
} else {
expand_fixed_length(store, rel, target_node, b, src, to_var, new_bindings,
&new_count, max_new, &match_count);
expand_fixed_length(store, rel, target_node, b, src, to_var, &new_bindings,
&new_count, &new_cap, &match_count);
}

/* OPTIONAL MATCH: no expansion for this source, so keep the binding
* with the target unbound (projection renders it ""). The buffer is
* sized max_new + *bind_count precisely so every such fallback row has
* a slot — no guard needed, and no OPTIONAL no-match row is dropped. */
* with the target unbound (projection renders it ""). The shared
* growable append gives every fallback row a slot. */
if (is_optional && match_count == 0) {
binding_t nb = {0};
binding_copy(&nb, b);
/* Don't set to_var — it remains unbound; projection returns "" */
new_bindings[new_count++] = nb;
(void)binding_out_append(&new_bindings, &new_count, &new_cap, &nb);
}
}

Expand All @@ -3337,6 +3353,7 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_
free(*bindings);
*bindings = new_bindings;
*bind_count = new_count;
*bind_cap = new_cap;
*var_name = to_var;
}
}
Expand Down Expand Up @@ -4577,8 +4594,8 @@ static void cross_join_with_rels(cbm_store_t *store, cbm_pattern_t *patn, bindin
* graphs (e.g. an unbound `c` scanned against ~29 K `f` bindings), wrapping
* the int product negative and yielding a tiny/garbage malloc → heap OOB
* write → SIGSEGV/SIGABRT (#627). */
size_t alloc_n = (size_t)*bind_count * (size_t)extra_count * (size_t)CYP_GROWTH_10 + SKIP_ONE;
binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t));
int new_cap = *bind_count > 0 && extra_count > 0 ? *bind_count : SKIP_ONE;
binding_t *new_bindings = malloc((size_t)new_cap * sizeof(binding_t));
if (!new_bindings) {
return; /* OOM: leave existing bindings untouched rather than corrupt */
}
Expand All @@ -4595,7 +4612,7 @@ static void cross_join_with_rels(cbm_store_t *store, cbm_pattern_t *patn, bindin
const char *tv = nvar;
expand_pattern_rels(store, patn, &tmp, &tc, &tcap, &tv, opt);
for (int ti = 0; ti < tc; ti++) {
new_bindings[new_count++] = tmp[ti];
(void)binding_out_append(&new_bindings, &new_count, &new_cap, &tmp[ti]);
}
free(tmp);
}
Expand Down Expand Up @@ -4647,13 +4664,12 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn,
* OPTIONAL no-match row (a data-loss bug, not an OOB — the fallback stayed
* in-bounds behind that guard) — exactly the rows
* `OPTIONAL MATCH ... WHERE <start> IS NULL` is meant to surface. */
size_t alloc_n = (size_t)*bind_count * (size_t)CYP_GROWTH_10 + (size_t)*bind_count;
binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t));
int new_cap = *bind_count > 0 ? *bind_count : CYP_GROWTH_10;
binding_t *new_bindings = malloc((size_t)new_cap * sizeof(binding_t));
if (!new_bindings) {
return;
}
int new_count = 0;
int max_new = *bind_count * CYP_GROWTH_10;

for (int bi = 0; bi < *bind_count; bi++) {
binding_t *b = &(*bindings)[bi];
Expand Down Expand Up @@ -4694,15 +4710,13 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn,
continue;
}
match_count++;
if (new_count < max_new) {
binding_t nb = {0};
binding_copy(&nb, b);
binding_set(&nb, start_var, &found);
if (rel->variable) {
binding_set_edge(&nb, rel->variable, &edges[ei]);
}
new_bindings[new_count++] = nb;
binding_t nb = {0};
binding_copy(&nb, b);
binding_set(&nb, start_var, &found);
if (rel->variable) {
binding_set_edge(&nb, rel->variable, &edges[ei]);
}
(void)binding_out_append(&new_bindings, &new_count, &new_cap, &nb);
node_fields_free(&found);
}
cbm_store_free_edges(edges, edge_count);
Expand All @@ -4711,15 +4725,12 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn,
if (opt && match_count == 0) {
/* GENUINELY no matching neighbour: the scan above runs unconditionally,
* so match_count is the true neighbour count and match_count == 0 here
* means the terminal really has none — not merely that the buffer filled
* first. Keep the row with start_var left UNBOUND
* so `WHERE <start> IS NULL` correctly identifies the no-edge case. The
* buffer is sized max_new + *bind_count precisely so every fallback row
* has a slot — no guard needed, and no OPTIONAL no-match row is dropped
* even after the expansion has saturated max_new. */
* means the terminal really has none. Keep the row with start_var left
* UNBOUND so `WHERE <start> IS NULL` correctly identifies the no-edge
* case; the shared growable append gives every fallback row a slot. */
binding_t nb = {0};
binding_copy(&nb, b);
new_bindings[new_count++] = nb;
(void)binding_out_append(&new_bindings, &new_count, &new_cap, &nb);
}
}

Expand Down
Loading
Loading