mirror of
https://github.com/DeusData/codebase-memory-mcp.git
synced 2026-11-04 10:48:20 +00:00
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 <ATHundt@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This commit is contained in:
1 parent
66d9c2be88
commit
dc5293d71e
3 files changed
+170
-8
No files matched your search
+91
-1
@@ -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 {
|
||||
|
||||
@@ -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"
|
||||
@@ -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);
|
||||
|
||||
Reference in new issue
Block a user