From 068690cabf357078911471994da36c37e99b5eb9 Mon Sep 17 00:00:00 2001 From: SEPURI-SAI-KRISHNA Date: Fri, 31 Jul 2026 15:27:20 +0530 Subject: [PATCH] fix(cypher): make expand_from_bound_terminal OPTIONAL fallback lossless Signed-off-by: SEPURI-SAI-KRISHNA --- src/cypher/cypher.c | 59 ++++++++++++----- tests/test_cypher.c | 150 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 16 deletions(-) diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index eba190777..e2483c8ff 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -4549,21 +4549,40 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn, bool scan_targets = !rel_inbound; /* (start)->(term): start = edge source = scan term's inbound */ - size_t alloc_n = (size_t)*bind_count * (size_t)CYP_GROWTH_10 + SKIP_ONE; + /* Size this hop's output for BOTH writers without dropping any row: the + * materialised expansion is capped at max_new = *bind_count * 10 rows (only + * the WRITE is gated on max_new; match DETECTION below is not, so a full + * buffer never hides a real neighbour), 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 "+ SKIP_ONE" sizing tied max_new to the whole buffer, so once + * the expansion saturated it the fallback guard silently dropped every later + * 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 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)); if (!new_bindings) { return; } int new_count = 0; - int max_new = (int)alloc_n; + int max_new = *bind_count * CYP_GROWTH_10; - for (int bi = 0; bi < *bind_count && new_count < max_new; bi++) { + for (int bi = 0; bi < *bind_count; bi++) { binding_t *b = &(*bindings)[bi]; cbm_node_t *term = binding_get(b, patn->nodes[1].variable ? patn->nodes[1].variable : ""); int match_count = 0; if (term) { - for (int ti = 0; - ti < (rel->type_count > 0 ? rel->type_count : 1) && new_count < max_new; ti++) { + /* Detection is decoupled from the write budget: scan every edge and + * type unconditionally so match_count reflects the true neighbour + * count, and gate only the WRITE on the ceiling (below). If detection + * stopped at max_new too, a saturated buffer would leave match_count at + * 0 for a terminal that actually has callers, and the fallback below + * would fabricate an UNBOUND "dead code" row for live code — worse than + * dropping a row. */ + for (int ti = 0; ti < (rel->type_count > 0 ? rel->type_count : 1); ti++) { cbm_edge_t *edges = NULL; int edge_count = 0; if (rel->type_count > 0) { @@ -4579,7 +4598,7 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn, } else { cbm_store_find_edges_by_source(store, term->id, &edges, &edge_count); } - for (int ei = 0; ei < edge_count && new_count < max_new; ei++) { + for (int ei = 0; ei < edge_count; ei++) { int64_t sid = scan_targets ? edges[ei].source_id : edges[ei].target_id; cbm_node_t found = {0}; if (cbm_store_find_node_by_id(store, sid, &found) != CBM_STORE_OK) { @@ -4589,22 +4608,30 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn, node_fields_free(&found); continue; } - 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]); + 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; } node_fields_free(&found); - new_bindings[new_count++] = nb; - match_count++; } cbm_store_free_edges(edges, edge_count); } } - if (opt && match_count == 0 && new_count < max_new) { - /* No matching neighbour: keep the row with start_var left UNBOUND so - * `WHERE IS NULL` correctly identifies the no-edge case. */ + 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 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. */ binding_t nb = {0}; binding_copy(&nb, b); new_bindings[new_count++] = nb; diff --git a/tests/test_cypher.c b/tests/test_cypher.c index 658ee5199..4e490ea36 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -675,6 +675,154 @@ TEST(cypher_exec_optional_rel_leaf_fallback_survives) { PASS(); } +/* Sibling of the leaf-fallback test, but for the BOUND-TERMINAL expansion path + * (expand_from_bound_terminal): the OPTIONAL start var is unbound and the + * terminal is bound, e.g. `MATCH (f) OPTIONAL MATCH (c)-[:CALLS]->(f)`. That + * function sized its hop buffer bind_count*10 + 1 and gated the OPTIONAL + * fallback on `new_count < max_new`, so once one terminal's expansion saturated + * the buffer, every LATER terminal's no-match row was silently dropped — the + * rows `WHERE c IS NULL` is meant to surface. Not an overflow (the guard kept + * the write in bounds) but real data loss. Lossless sizing (bind_count*10 + + * bind_count, fallback ungated) preserves them. */ +TEST(cypher_exec_bound_terminal_optional_fallback_survives) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + + /* 2 Function nodes (hub + leaf) → bind_count = 2, max_new = 20. The hub has + * 21 incoming CALLS edges (> max_new), so its expansion saturates the write + * buffer; the leaf has none, so it must still yield its OPTIONAL no-match + * row. Under the original "+ SKIP_ONE" sizing that row was dropped once the + * buffer filled; the lossless sizing preserves it regardless of the order in + * which the (unordered) label scan visits the two terminals. */ + cbm_node_t hub = { + .project = "test", .label = "Function", .name = "hub", .qualified_name = "test.hub"}; + int64_t hub_id = cbm_store_upsert_node(s, &hub); + cbm_node_t leaf = { + .project = "test", .label = "Function", .name = "leaf", .qualified_name = "test.leaf"}; + cbm_store_upsert_node(s, &leaf); + + /* Callers are non-Function so they do not inflate bind_count. Each CALLS the + * hub (source = caller, target = hub), so from the bound terminal `hub` the + * expansion binds the unbound start `c` to each caller. */ + for (int i = 0; i < 21; i++) { + char nm[32]; + char qn[48]; + snprintf(nm, sizeof(nm), "caller%02d", i); + snprintf(qn, sizeof(qn), "test.caller%02d", i); + cbm_node_t caller = {.project = "test", .label = "Var", .name = nm, .qualified_name = qn}; + int64_t cid = cbm_store_upsert_node(s, &caller); + cbm_edge_t e = {.project = "test", .source_id = cid, .target_id = hub_id, .type = "CALLS"}; + cbm_store_insert_edge(s, &e); + } + + /* max_rows 0 → the 100000 result ceiling, so the output LIMIT does not hide + * the leaf row; we are testing the hop buffer, not the output cap. */ + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute( + s, "MATCH (f:Function) OPTIONAL MATCH (c)-[:CALLS]->(f) RETURN f.name, c.name", "test", 0, + &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.col_count, 2); + + /* The leaf terminal has no incoming CALLS edge, so it must yield one row with + * the start var `c` unbound (""). Before the fix this row was dropped once the + * hub saturated the buffer. Also confirm the hub still expanded to bound rows. */ + bool leaf_fallback = false; + bool hub_expanded = false; + for (int i = 0; i < r.row_count; i++) { + const char *f = r.rows[i][0]; + const char *c = r.rows[i][1]; + if (strcmp(f, "leaf") == 0 && c[0] == '\0') { + leaf_fallback = true; + } + if (strcmp(f, "hub") == 0 && c[0] != '\0') { + hub_expanded = true; + } + } + ASSERT_TRUE(leaf_fallback); /* the bound-terminal OPTIONAL no-match row survived */ + ASSERT_TRUE(hub_expanded); /* the expansion still produced bound rows */ + + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +/* Discriminating companion to the test above: preserving OPTIONAL no-match rows + * under saturation must NOT be bought by fabricating them. If match detection is + * gated on the same ceiling as the write, then once one terminal fills the + * buffer, another terminal that genuinely HAS callers is never scanned, its + * match_count stays 0, and the fallback invents an unbound "dead code" row for it + * — reporting live code as dead, which is worse than dropping a row. + * + * The construction is deliberately order-independent: BOTH hubs have enough + * callers to saturate the buffer on their own, so whichever the scan visits + * second is guaranteed to be processed after saturation. Under the gated variant + * that second hub is fabricated as dead; the assertion "no hub with callers is + * dead" then fails no matter which order `find_nodes_by_label` returns (its query + * has no ORDER BY, so the test must not depend on one). The fix makes it pass. */ +TEST(cypher_exec_bound_terminal_saturation_no_false_deadcode) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + + /* 3 Function terminals → bind_count = 3, max_new = 30. */ + cbm_node_t hubA = { + .project = "test", .label = "Function", .name = "hubA", .qualified_name = "test.hubA"}; + int64_t hubA_id = cbm_store_upsert_node(s, &hubA); + cbm_node_t hubB = { + .project = "test", .label = "Function", .name = "hubB", .qualified_name = "test.hubB"}; + int64_t hubB_id = cbm_store_upsert_node(s, &hubB); + cbm_node_t leaf = { + .project = "test", .label = "Function", .name = "leaf", .qualified_name = "test.leaf"}; + cbm_store_upsert_node(s, &leaf); + + /* BOTH hubs get 35 callers (> max_new = 30), so either one saturates the write + * buffer by itself; leaf gets none. Callers are non-Function so they don't + * inflate bind_count. */ + for (int i = 0; i < 70; i++) { + char nm[32]; + char qn[48]; + snprintf(nm, sizeof(nm), "caller%02d", i); + snprintf(qn, sizeof(qn), "test.caller%02d", i); + cbm_node_t caller = {.project = "test", .label = "Var", .name = nm, .qualified_name = qn}; + int64_t cid = cbm_store_upsert_node(s, &caller); + int64_t tgt = i < 35 ? hubA_id : hubB_id; /* 35 -> hubA, 35 -> hubB */ + cbm_edge_t e = {.project = "test", .source_id = cid, .target_id = tgt, .type = "CALLS"}; + cbm_store_insert_edge(s, &e); + } + + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute( + s, "MATCH (f:Function) OPTIONAL MATCH (c)-[:CALLS]->(f) RETURN f.name, c.name", "test", 0, + &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.col_count, 2); + + bool hub_expanded = false; /* sanity: the buffer really did fill from a hub */ + bool hub_false_deadcode = false; /* the bug: a hub with callers invented as dead */ + bool leaf_deadcode = false; /* the lossless property: genuine dead code kept */ + for (int i = 0; i < r.row_count; i++) { + const char *f = r.rows[i][0]; + const char *c = r.rows[i][1]; + bool is_hub = strcmp(f, "hubA") == 0 || strcmp(f, "hubB") == 0; + if (is_hub && c[0] != '\0') { + hub_expanded = true; + } + if (is_hub && c[0] == '\0') { + hub_false_deadcode = true; + } + if (strcmp(f, "leaf") == 0 && c[0] == '\0') { + leaf_deadcode = true; + } + } + ASSERT_TRUE(hub_expanded); + ASSERT_FALSE(hub_false_deadcode); /* live code with callers must never appear as dead */ + ASSERT_TRUE(leaf_deadcode); /* genuine no-match row still survives saturation */ + + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + TEST(cypher_exec_where_eq) { cbm_store_t *s = setup_cypher_store(); cbm_cypher_result_t r = {0}; @@ -3425,6 +3573,8 @@ SUITE(cypher) { RUN_TEST(cypher_cross_join_alloc_rejects_overflow); RUN_TEST(cypher_exec_optional_rel_saturated_no_overflow); RUN_TEST(cypher_exec_optional_rel_leaf_fallback_survives); + RUN_TEST(cypher_exec_bound_terminal_optional_fallback_survives); + RUN_TEST(cypher_exec_bound_terminal_saturation_no_false_deadcode); RUN_TEST(cypher_issue240_labels_function); RUN_TEST(cypher_issue237_distinct_order_limit); RUN_TEST(cypher_issue873_distinct_order_limit_dedupes_before_limit);