Skip to content

Commit d6be58e

Browse files
authored
Merge pull request DeusData#1385 from DeusData/fix/cypher-optional-no-fabrication
fix(cypher): never fabricate an OPTIONAL "no match" after the budget fills
2 parents 2789301 + 27d9831 commit d6be58e

2 files changed

Lines changed: 111 additions & 13 deletions

File tree

‎src/cypher/cypher.c‎

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3082,7 +3082,15 @@ static void process_edges(cbm_store_t *store, cbm_edge_t *edges, int edge_count,
30823082
* the result of dead-code queries and produced wrong rows (#627). */
30833083
cbm_node_t *bound_to = binding_get(b, to_var);
30843084
int64_t bound_to_id = bound_to ? bound_to->id : 0;
3085-
for (int ei = 0; ei < edge_count && *new_count < max_new; ei++) {
3085+
/* The budget caps MATERIALISATION, not detection: `match_count` must stay
3086+
* truthful even after `new_count` hits `max_new`. Gating the loop itself
3087+
* (`ei < edge_count && *new_count < max_new`) stopped fetching entirely, so
3088+
* a saturated source reported `match_count == 0` despite having neighbours —
3089+
* and the OPTIONAL fallback in expand_pattern_rels then emitted an unbound
3090+
* "no match" row for it. `WHERE x IS NULL` reads those rows as "nothing
3091+
* points here", i.e. it reported live code as dead. Losing rows is
3092+
* recoverable; asserting a match does not exist when it does is not. */
3093+
for (int ei = 0; ei < edge_count; ei++) {
30863094
int64_t tid = inbound ? edges[ei].source_id : edges[ei].target_id;
30873095
if (bound_to && tid != bound_to_id) {
30883096
continue; /* edge does not reach the already-bound terminal node */
@@ -3099,15 +3107,17 @@ static void process_edges(cbm_store_t *store, cbm_edge_t *edges, int edge_count,
30993107
node_fields_free(&found);
31003108
continue;
31013109
}
3102-
binding_t nb = {0};
3103-
binding_copy(&nb, b);
3104-
binding_set(&nb, to_var, &found);
3105-
if (rel_var) {
3106-
binding_set_edge(&nb, rel_var, &edges[ei]);
3110+
(*match_count)++; /* a real neighbour exists, budget or not */
3111+
if (*new_count < max_new) {
3112+
binding_t nb = {0};
3113+
binding_copy(&nb, b);
3114+
binding_set(&nb, to_var, &found);
3115+
if (rel_var) {
3116+
binding_set_edge(&nb, rel_var, &edges[ei]);
3117+
}
3118+
new_bindings[(*new_count)++] = nb;
31073119
}
31083120
node_fields_free(&found);
3109-
new_bindings[(*new_count)++] = nb;
3110-
(*match_count)++;
31113121
}
31123122
}
31133123

@@ -3141,7 +3151,10 @@ static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
31413151
cbm_traverse_result_t tr = {0};
31423152
const char *dir = rel->direction ? rel->direction : "outbound";
31433153
cbm_store_bfs(store, src->id, dir, rel->types, rel->type_count, max_depth, CBM_PERCENT, &tr);
3144-
for (int v = 0; v < tr.visited_count && *new_count < max_new; v++) {
3154+
/* Same contract as process_edges: the budget caps materialisation, never
3155+
* detection, so a saturated source cannot report match_count == 0 and get a
3156+
* fabricated OPTIONAL "no match" row. */
3157+
for (int v = 0; v < tr.visited_count; v++) {
31453158
cbm_node_hop_t *hop = &tr.visited[v];
31463159
if (hop->hop < rel->min_hops) {
31473160
continue;
@@ -3152,11 +3165,13 @@ static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
31523165
if (!check_inline_props(&hop->node, target_node->props, target_node->prop_count, store)) {
31533166
continue;
31543167
}
3155-
binding_t nb = {0};
3156-
binding_copy(&nb, b);
3157-
binding_set(&nb, to_var, &hop->node);
3158-
new_bindings[(*new_count)++] = nb;
31593168
(*match_count)++;
3169+
if (*new_count < max_new) {
3170+
binding_t nb = {0};
3171+
binding_copy(&nb, b);
3172+
binding_set(&nb, to_var, &hop->node);
3173+
new_bindings[(*new_count)++] = nb;
3174+
}
31603175
}
31613176
cbm_store_traverse_free(&tr);
31623177
}

‎tests/test_cypher.c‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -580,6 +580,88 @@ TEST(cypher_exec_optional_rel_saturated_no_overflow) {
580580
PASS();
581581
}
582582

583+
/* Reproduce-first: after the expansion budget is exhausted, OPTIONAL MATCH must
584+
* not FABRICATE a "no match" row for a source that genuinely has matches.
585+
*
586+
* process_edges / expand_var_length used to carry the budget in the LOOP
587+
* condition (`ei < edge_count && *new_count < max_new`), so once new_count hit
588+
* max_new they stopped iterating entirely and never incremented match_count —
589+
* even though neighbours existed. expand_pattern_rels' ungated fallback then saw
590+
* match_count == 0 and emitted an unbound row. `WHERE b IS NULL` reads that as
591+
* "nothing points here", so a dead-code query reported LIVE code as dead.
592+
*
593+
* Shape: A saturates the budget, B genuinely has no callees, C has 5. Only B may
594+
* appear with an unbound b. Asserting on C specifically is what discriminates —
595+
* the pre-fix code emits C with an empty b, and a `row_count` check would not
596+
* notice. Deterministic: insertion order fixes the scan order (rowid), and every
597+
* count is exact. */
598+
TEST(cypher_exec_optional_saturated_does_not_fabricate_no_match) {
599+
cbm_store_t *s = cbm_store_open_memory();
600+
cbm_store_upsert_project(s, "test", "/tmp/test");
601+
602+
/* 3 Function nodes → scan_count = 3; max_rows = 3 → bind_cap = 3 and
603+
* max_new = 30. A alone exceeds that, so B and C are reached with the
604+
* budget already spent — the regime that produced the fabrication. */
605+
cbm_node_t a = {.project = "test", .label = "Function", .name = "A", .qualified_name = "test.A"};
606+
cbm_node_t b = {.project = "test", .label = "Function", .name = "B", .qualified_name = "test.B"};
607+
cbm_node_t c = {.project = "test", .label = "Function", .name = "C", .qualified_name = "test.C"};
608+
int64_t a_id = cbm_store_upsert_node(s, &a);
609+
(void)cbm_store_upsert_node(s, &b); /* B: no outgoing CALLS at all */
610+
int64_t c_id = cbm_store_upsert_node(s, &c);
611+
ASSERT_GT(a_id, 0);
612+
ASSERT_GT(c_id, 0);
613+
614+
/* Callees are label Var so they do not inflate scan_count/bind_cap. */
615+
for (int i = 0; i < 40; i++) {
616+
char nm[32];
617+
char qn[48];
618+
snprintf(nm, sizeof(nm), "acallee%d", i);
619+
snprintf(qn, sizeof(qn), "test.acallee%d", i);
620+
cbm_node_t t = {.project = "test", .label = "Var", .name = nm, .qualified_name = qn};
621+
int64_t tid = cbm_store_upsert_node(s, &t);
622+
cbm_edge_t e = {.project = "test", .source_id = a_id, .target_id = tid, .type = "CALLS"};
623+
cbm_store_insert_edge(s, &e);
624+
}
625+
for (int i = 0; i < 5; i++) {
626+
char nm[32];
627+
char qn[48];
628+
snprintf(nm, sizeof(nm), "ccallee%d", i);
629+
snprintf(qn, sizeof(qn), "test.ccallee%d", i);
630+
cbm_node_t t = {.project = "test", .label = "Var", .name = nm, .qualified_name = qn};
631+
int64_t tid = cbm_store_upsert_node(s, &t);
632+
cbm_edge_t e = {.project = "test", .source_id = c_id, .target_id = tid, .type = "CALLS"};
633+
cbm_store_insert_edge(s, &e);
634+
}
635+
636+
cbm_cypher_result_t r = {0};
637+
int rc = cbm_cypher_execute(
638+
s, "MATCH (f:Function) OPTIONAL MATCH (f)-[:CALLS]->(g) WHERE g IS NULL RETURN f.name",
639+
"test", 3, &r);
640+
ASSERT_EQ(rc, 0);
641+
642+
/* Positive control: B genuinely has no callees, so the query must still find
643+
* it. Without this a "C is absent" assertion could pass on an empty result. */
644+
bool saw_b = false;
645+
bool saw_c = false;
646+
for (int i = 0; i < r.row_count; i++) {
647+
const char *name = r.rows[i][0];
648+
if (name && strcmp(name, "B") == 0) {
649+
saw_b = true;
650+
}
651+
if (name && strcmp(name, "C") == 0) {
652+
saw_c = true;
653+
}
654+
}
655+
ASSERT_TRUE(saw_b);
656+
/* The discriminator: C has 5 callees, so claiming it has none is a
657+
* fabrication. Pre-fix this is exactly what the saturated path emitted. */
658+
ASSERT_FALSE(saw_c);
659+
660+
cbm_cypher_result_free(&r);
661+
cbm_store_close(s);
662+
PASS();
663+
}
664+
583665
/* Arithmetic-boundary companion to the zero-label overflow above: the node
584666
* cross-join sizes its buffer from bind_count * extra_count. As a plain int that
585667
* product wraps past INT_MAX to a negative/garbage malloc size (the large-graph
@@ -3491,6 +3573,7 @@ SUITE(cypher) {
34913573
RUN_TEST(cypher_exec_optional_empty_label_no_overflow);
34923574
RUN_TEST(cypher_cross_join_alloc_rejects_overflow);
34933575
RUN_TEST(cypher_exec_optional_rel_saturated_no_overflow);
3576+
RUN_TEST(cypher_exec_optional_saturated_does_not_fabricate_no_match);
34943577
RUN_TEST(cypher_exec_optional_rel_leaf_fallback_survives);
34953578
RUN_TEST(cypher_issue240_labels_function);
34963579
RUN_TEST(cypher_issue237_distinct_order_limit);

0 commit comments

Comments
 (0)