From dc5293d71e705604d315f122a60b92f0176ff360 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 20 Sep 2026 17:32:12 +0200 Subject: [PATCH] fix(cypher): stop the early WHERE pruning rows it cannot judge yet Distilled from #1245 by Andrew Hundt (upstream b835b5e8), re-derived on main's cypher.c rather than cherry-picked -- the file has moved ~9k lines since the branch point. The WHERE clause runs twice: once early, on seed rows where only the first alias is bound, to prune; and once late, when every alias is bound, to decide. The early pass used the same two-valued evaluator as the late one. That evaluator answers "true" for a comparison on an alias it cannot see -- the permissive default that makes the late pass work -- but NOT and XOR invert whatever they are given, so MATCH (a)-[:CALLS]->(b) WHERE NOT b.name CONTAINS "x" evaluated NOT(true) on every seed and pruned every row. Zero results, no error, for a query that is correct. The early pass now uses a three-valued evaluator. A condition on an alias that is not yet bound is UNKNOWN; NOT of UNKNOWN stays UNKNOWN; AND and OR short-circuit only on a definite operand; XOR with any UNKNOWN operand is UNKNOWN. Only a definite FALSE prunes. The late pass is unchanged and still gives the final two-valued answer, so nothing that was correct before changes. Three deliberate differences from the upstream diff: - the unbound-alias rule lives in one leaf, eval_condition_partial, so the expression-tree path and main's legacy flat AND/OR path cannot drift apart; - that leaf guards c->variable == NULL. On main a condition whose left side is a function of literals, coalesce(0, 1) >= 2, carries no variable at all; the upstream version would have passed NULL to strcmp; - the flat path keeps main's rule that an empty condition list is true. RED before the fix, all four with row_count == 0, expected 1: cypher_exec_where_not_on_relationship_target, cypher_exec_where_mixed_alias_and, ..._or, ..._xor. GREEN after: cypher 208 passed; cypher + mcp 526 passed, 4 skipped (the Windows-only skips). eval_expr_partial recurses and is added to CBM_RECURSION_WHITELIST. Co-authored-by: Andrew Hundt Signed-off-by: Martin Vogel --- src/cypher/cypher.c | 92 +++++++++++++++++++++++++++- src/foundation/recursion_whitelist.h | 14 ++--- tests/test_cypher.c | 72 ++++++++++++++++++++++ 3 files changed, 170 insertions(+), 8 deletions(-) diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 234f49087..8bf69af24 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -2803,6 +2803,96 @@ static bool eval_where(const cbm_where_clause_t *w, binding_t *b) { return is_and; } +/* Three-valued WHERE evaluation for the early (seed-row) pass in + * execute_single: only the first MATCH alias is bound there, so a leaf on + * any other alias is UNKNOWN rather than true. The two-valued evaluator + * returns true for an unbound alias, which NOT / XOR then invert -- pruning + * every seed before relationship expansion (distilled from PR #1245). The + * full evaluation reruns after expansion (Step 3), so the early pass only has + * to prune on a definite false. Allocation-free, O(expression nodes). */ +typedef enum { CYP_PARTIAL_FALSE = 0, CYP_PARTIAL_TRUE, CYP_PARTIAL_UNKNOWN } cyp_partial_t; + +static cyp_partial_t partial_and(cyp_partial_t l, cyp_partial_t r) { + if (l == CYP_PARTIAL_FALSE || r == CYP_PARTIAL_FALSE) { + return CYP_PARTIAL_FALSE; + } + return (l == CYP_PARTIAL_TRUE && r == CYP_PARTIAL_TRUE) ? CYP_PARTIAL_TRUE + : CYP_PARTIAL_UNKNOWN; +} + +static cyp_partial_t partial_or(cyp_partial_t l, cyp_partial_t r) { + if (l == CYP_PARTIAL_TRUE || r == CYP_PARTIAL_TRUE) { + return CYP_PARTIAL_TRUE; + } + return (l == CYP_PARTIAL_FALSE && r == CYP_PARTIAL_FALSE) ? CYP_PARTIAL_FALSE + : CYP_PARTIAL_UNKNOWN; +} + +/* A condition whose alias is not bound yet is UNKNOWN; a literal-only LHS + * (func condition with no variable arg) has nothing to wait for. */ +static cyp_partial_t eval_condition_partial(const cbm_condition_t *c, binding_t *b) { + if (c->variable && !binding_get(b, c->variable) && !binding_get_edge(b, c->variable)) { + return CYP_PARTIAL_UNKNOWN; + } + return eval_condition(c, b) ? CYP_PARTIAL_TRUE : CYP_PARTIAL_FALSE; +} + +static cyp_partial_t eval_expr_partial(const cbm_expr_t *e, // NOLINT(misc-no-recursion) + binding_t *b) { + if (!e) { + return CYP_PARTIAL_TRUE; + } + if (e->type == EXPR_CONDITION) { + return eval_condition_partial(&e->cond, b); + } + cyp_partial_t left = eval_expr_partial(e->left, b); + if (e->type == EXPR_NOT) { + if (left == CYP_PARTIAL_UNKNOWN) { + return CYP_PARTIAL_UNKNOWN; + } + return left == CYP_PARTIAL_TRUE ? CYP_PARTIAL_FALSE : CYP_PARTIAL_TRUE; + } + /* Same short-circuits as eval_expr: a definite left decides AND / OR. */ + if (e->type == EXPR_AND && left == CYP_PARTIAL_FALSE) { + return CYP_PARTIAL_FALSE; + } + if (e->type == EXPR_OR && left == CYP_PARTIAL_TRUE) { + return CYP_PARTIAL_TRUE; + } + cyp_partial_t right = eval_expr_partial(e->right, b); + if (e->type == EXPR_AND) { + return partial_and(left, right); + } + if (e->type == EXPR_OR) { + return partial_or(left, right); + } + /* EXPR_XOR */ + if (left == CYP_PARTIAL_UNKNOWN || right == CYP_PARTIAL_UNKNOWN) { + return CYP_PARTIAL_UNKNOWN; + } + return left != right ? CYP_PARTIAL_TRUE : CYP_PARTIAL_FALSE; +} + +static cyp_partial_t eval_where_partial(const cbm_where_clause_t *w, binding_t *b) { + if (!w) { + return CYP_PARTIAL_TRUE; + } + if (w->root) { + return eval_expr_partial(w->root, b); + } + /* Legacy flat evaluation */ + if (w->count == 0) { + return CYP_PARTIAL_TRUE; + } + bool is_and = (w->op && strcmp(w->op, "AND") == 0) != 0; + cyp_partial_t result = is_and ? CYP_PARTIAL_TRUE : CYP_PARTIAL_FALSE; + for (int i = 0; i < w->count; i++) { + cyp_partial_t r = eval_condition_partial(&w->conditions[i], b); + result = is_and ? partial_and(result, r) : partial_or(result, r); + } + return result; +} + /* Check if a string value looks like a regex pattern. */ static bool looks_like_regex(const char *s) { if (!s) { @@ -5135,7 +5225,7 @@ static int execute_single(cbm_store_t *store, cbm_query_t *q, const char *projec binding_t b = {0}; b.store = store; binding_set(&b, var_name, &scanned[i]); - bool pass = !q->where || eval_where(q->where, &b); + bool pass = eval_where_partial(q->where, &b) != CYP_PARTIAL_FALSE; if (pass) { bindings[bind_count++] = b; } else { diff --git a/src/foundation/recursion_whitelist.h b/src/foundation/recursion_whitelist.h index c3e2b5c8c..73a38cc50 100644 --- a/src/foundation/recursion_whitelist.h +++ b/src/foundation/recursion_whitelist.h @@ -8,8 +8,8 @@ * - parse_or_expr, parse_xor_expr, parse_and_expr, parse_not_expr * - parse_atom_expr, parse_post_where, cbm_parse * - * Cypher expression evaluator (bounded by WHERE clause depth ~5): - * - eval_expr + * Cypher expression evaluators (bounded by WHERE clause depth ~5): + * - eval_expr, eval_expr_partial * * Glob pattern matcher (bounded by pattern nesting ~3): * - glob_match, glob_match_star, glob_match_doublestar @@ -25,9 +25,9 @@ * To add a function: add it below AND add NOLINT(misc-no-recursion) on * the function definition line. The lint gate verifies both match. */ -#define CBM_RECURSION_WHITELIST \ - "parse_or_expr", "parse_xor_expr", "parse_and_expr", "parse_not_expr", "parse_atom_expr", \ - "parse_post_where", "cbm_parse", "eval_expr", "glob_match", "glob_match_star", \ - "glob_match_doublestar", "glob_match_doublestar_slash", "glob_match_doublestar_any", \ - "parse_bool_expr", "parse_bool_atom", "r_collect_imports", \ +#define CBM_RECURSION_WHITELIST \ + "parse_or_expr", "parse_xor_expr", "parse_and_expr", "parse_not_expr", "parse_atom_expr", \ + "parse_post_where", "cbm_parse", "eval_expr", "eval_expr_partial", "glob_match", \ + "glob_match_star", "glob_match_doublestar", "glob_match_doublestar_slash", \ + "glob_match_doublestar_any", "parse_bool_expr", "parse_bool_atom", "r_collect_imports", \ "find_first_descendant_by_kind", "find_first_descendant_of" diff --git a/tests/test_cypher.c b/tests/test_cypher.c index 8bdee76c9..853abc266 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -3111,6 +3111,74 @@ TEST(cypher_exec_where_not) { PASS(); } +/* Regression (distilled from PR #1245, Andrew Hundt): the early WHERE pass in + * execute_single runs on seed rows with only the first alias bound. A + * comparison on an unbound alias used to evaluate to true, which NOT / XOR + * then inverted -- so every seed was pruned before CALLS expansion and the + * query returned zero rows. Unbound leaves must stay UNKNOWN there. */ +TEST(cypher_exec_where_not_on_relationship_target) { + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, + "MATCH (a:Function)-[:CALLS]->(b:Function) " + "WHERE NOT b.name CONTAINS \"Order\" RETURN b.name", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], "LogError"); + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +TEST(cypher_exec_where_mixed_alias_and) { + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute( + s, + "MATCH (a:Function)-[:CALLS]->(b:Function) " + "WHERE a.name = \"HandleOrder\" AND NOT b.name CONTAINS \"Order\" RETURN b.name", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], "LogError"); + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +TEST(cypher_exec_where_mixed_alias_or) { + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute( + s, + "MATCH (a:Function)-[:CALLS]->(b:Function) " + "WHERE a.name = \"NoSuchFunction\" OR NOT b.name CONTAINS \"Order\" RETURN b.name", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], "LogError"); + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +TEST(cypher_exec_where_mixed_alias_xor) { + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = + cbm_cypher_execute(s, + "MATCH (a:Function)-[:CALLS]->(b:Function) " + "WHERE a.name = \"HandleOrder\" XOR b.name = \"LogError\" RETURN b.name", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], "ValidateOrder"); + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + TEST(cypher_exec_where_in) { cbm_store_t *s = setup_cypher_store(); cbm_cypher_result_t r = {0}; @@ -4769,6 +4837,10 @@ SUITE(cypher) { RUN_TEST(cypher_exec_where_neq_bang); RUN_TEST(cypher_exec_where_ends_with); RUN_TEST(cypher_exec_where_not); + RUN_TEST(cypher_exec_where_not_on_relationship_target); + RUN_TEST(cypher_exec_where_mixed_alias_and); + RUN_TEST(cypher_exec_where_mixed_alias_or); + RUN_TEST(cypher_exec_where_mixed_alias_xor); RUN_TEST(cypher_exec_where_in); RUN_TEST(cypher_exec_where_not_in); RUN_TEST(cypher_exec_where_is_null);