Skip to content

Commit 8f35274

Browse files
ngallodevclaude
andauthored
fix(cypher): preserve partial WHERE and optional COUNT semantics (#5)
## Summary - Preserve three-valued `WHERE` evaluation while only the seed alias is bound, so later aliases are not pruned early. - Make `COUNT(variable)` ignore unbound optional matches. - Adapt upstream Cypher work `458b0db1` through `6166934d`. ## Provenance and gates - Agent Run: `luna-cbm-cypher-20260920-r1`; source revision `a43b2201`; worker commit `42ca613c`. - Agent-Workflow reports a valid completion receipt. Independent review, evaluation, acceptance, and integration are pending. - `git diff --check main...HEAD` passed. No tests were run for this PR request. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * WHERE filters now handle conditions involving relationship matches more accurately, including combinations of `AND`, `OR`, `NOT`, and `XOR`. * `COUNT(variable)` now excludes rows where the variable is unbound, including after a `WITH` clause. `COUNT(*)` continues to count all rows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Review follow-up Addressed in 89e3ace. 1. **COUNT in WITH aggregation**: fixed. `binding_has_value` now sits above both accumulators, and `with_agg_accumulate` skips COUNT under the same condition as the RETURN path. New test `cypher_exec_with_optional_bound_terminal_count_ignores_unbound` checks that HandleOrder gets 0. 2. **Multi-alias function conditions in the early pass**: fixed. `eval_condition_partial` returns UNKNOWN when any function argument references an unbound variable, including when the first argument is a literal. New test `cypher_exec_where_coalesce_multi_alias` uses `coalesce(a.nosuch, b.name) = "LogError"`. Both new tests fail without the cypher.c change (198 passed, 2 failed) and pass with it. `scripts/test.sh --suites cypher`: 200 passed. `make lint-ci lint-tidy-diff` passes. Not changed: `COUNT(t.prop)` still counts a bound node whose property is missing. As noted in the review, this is not a regression. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 72faa8c commit 8f35274

3 files changed

Lines changed: 285 additions & 11 deletions

File tree

‎src/cypher/cypher.c‎

Lines changed: 129 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2750,6 +2750,103 @@ static bool eval_where(const cbm_where_clause_t *w, binding_t *b) {
27502750
return is_and;
27512751
}
27522752

2753+
/* Three-valued WHERE evaluation for the early (seed-row) pass in
2754+
* execute_single: only the first MATCH alias is bound there, so a leaf on
2755+
* any other alias is UNKNOWN rather than true. The two-valued evaluator
2756+
* returns true for an unbound alias, which NOT / XOR then invert -- pruning
2757+
* every seed before relationship expansion (distilled from PR #1245). The
2758+
* full evaluation reruns after expansion (Step 3), so the early pass only has
2759+
* to prune on a definite false. Allocation-free, O(expression nodes). */
2760+
typedef enum { CYP_PARTIAL_FALSE = 0, CYP_PARTIAL_TRUE, CYP_PARTIAL_UNKNOWN } cyp_partial_t;
2761+
2762+
static cyp_partial_t partial_and(cyp_partial_t l, cyp_partial_t r) {
2763+
if (l == CYP_PARTIAL_FALSE || r == CYP_PARTIAL_FALSE) {
2764+
return CYP_PARTIAL_FALSE;
2765+
}
2766+
return (l == CYP_PARTIAL_TRUE && r == CYP_PARTIAL_TRUE) ? CYP_PARTIAL_TRUE
2767+
: CYP_PARTIAL_UNKNOWN;
2768+
}
2769+
2770+
static cyp_partial_t partial_or(cyp_partial_t l, cyp_partial_t r) {
2771+
if (l == CYP_PARTIAL_TRUE || r == CYP_PARTIAL_TRUE) {
2772+
return CYP_PARTIAL_TRUE;
2773+
}
2774+
return (l == CYP_PARTIAL_FALSE && r == CYP_PARTIAL_FALSE) ? CYP_PARTIAL_FALSE
2775+
: CYP_PARTIAL_UNKNOWN;
2776+
}
2777+
2778+
/* A condition whose alias is not bound yet is UNKNOWN; a literal-only LHS
2779+
* (func condition with no variable arg) has nothing to wait for. */
2780+
static cyp_partial_t eval_condition_partial(const cbm_condition_t *c, binding_t *b) {
2781+
if (c->variable && !binding_get(b, c->variable) && !binding_get_edge(b, c->variable)) {
2782+
return CYP_PARTIAL_UNKNOWN;
2783+
}
2784+
/* Multi-arg function: any unbound variable arg makes it UNKNOWN. */
2785+
for (int i = 0; c->func && i < c->arg_count; i++) {
2786+
const char *av = c->args[i].variable;
2787+
if (av && !binding_get(b, av) && !binding_get_edge(b, av)) {
2788+
return CYP_PARTIAL_UNKNOWN;
2789+
}
2790+
}
2791+
return eval_condition(c, b) ? CYP_PARTIAL_TRUE : CYP_PARTIAL_FALSE;
2792+
}
2793+
2794+
static cyp_partial_t eval_expr_partial(const cbm_expr_t *e, // NOLINT(misc-no-recursion)
2795+
binding_t *b) {
2796+
if (!e) {
2797+
return CYP_PARTIAL_TRUE;
2798+
}
2799+
if (e->type == EXPR_CONDITION) {
2800+
return eval_condition_partial(&e->cond, b);
2801+
}
2802+
cyp_partial_t left = eval_expr_partial(e->left, b);
2803+
if (e->type == EXPR_NOT) {
2804+
if (left == CYP_PARTIAL_UNKNOWN) {
2805+
return CYP_PARTIAL_UNKNOWN;
2806+
}
2807+
return left == CYP_PARTIAL_TRUE ? CYP_PARTIAL_FALSE : CYP_PARTIAL_TRUE;
2808+
}
2809+
/* Same short-circuits as eval_expr: a definite left decides AND / OR. */
2810+
if (e->type == EXPR_AND && left == CYP_PARTIAL_FALSE) {
2811+
return CYP_PARTIAL_FALSE;
2812+
}
2813+
if (e->type == EXPR_OR && left == CYP_PARTIAL_TRUE) {
2814+
return CYP_PARTIAL_TRUE;
2815+
}
2816+
cyp_partial_t right = eval_expr_partial(e->right, b);
2817+
if (e->type == EXPR_AND) {
2818+
return partial_and(left, right);
2819+
}
2820+
if (e->type == EXPR_OR) {
2821+
return partial_or(left, right);
2822+
}
2823+
/* EXPR_XOR */
2824+
if (left == CYP_PARTIAL_UNKNOWN || right == CYP_PARTIAL_UNKNOWN) {
2825+
return CYP_PARTIAL_UNKNOWN;
2826+
}
2827+
return left != right ? CYP_PARTIAL_TRUE : CYP_PARTIAL_FALSE;
2828+
}
2829+
2830+
static cyp_partial_t eval_where_partial(const cbm_where_clause_t *w, binding_t *b) {
2831+
if (!w) {
2832+
return CYP_PARTIAL_TRUE;
2833+
}
2834+
if (w->root) {
2835+
return eval_expr_partial(w->root, b);
2836+
}
2837+
/* Legacy flat evaluation */
2838+
if (w->count == 0) {
2839+
return CYP_PARTIAL_TRUE;
2840+
}
2841+
bool is_and = (w->op && strcmp(w->op, "AND") == 0) != 0;
2842+
cyp_partial_t result = is_and ? CYP_PARTIAL_TRUE : CYP_PARTIAL_FALSE;
2843+
for (int i = 0; i < w->count; i++) {
2844+
cyp_partial_t r = eval_condition_partial(&w->conditions[i], b);
2845+
result = is_and ? partial_and(result, r) : partial_or(result, r);
2846+
}
2847+
return result;
2848+
}
2849+
27532850
/* Check if a string value looks like a regex pattern. */
27542851
static bool looks_like_regex(const char *s) {
27552852
if (!s) {
@@ -3751,6 +3848,10 @@ static void distinct_list_add(char ***list, int *count, const char *val) {
37513848
}
37523849
int idx = (*count)++;
37533850
*list = safe_realloc(*list, (size_t)(idx + SKIP_ONE) * sizeof(char *));
3851+
if (!*list) {
3852+
*count = 0; /* safe_realloc freed the old list */
3853+
return;
3854+
}
37543855
(*list)[idx] = heap_strdup(val);
37553856
}
37563857

@@ -3908,12 +4009,35 @@ static int with_agg_find_or_create(with_agg_t **aggs, int *agg_cnt, int *agg_cap
39084009
return found;
39094010
}
39104011

4012+
/* True when the aggregate's argument is bound (COUNT skips unbound OPTIONAL). */
4013+
static bool binding_has_value(binding_t *b, const char *var, const char *prop) {
4014+
if (!var || strcmp(var, "*") == 0) {
4015+
return true;
4016+
}
4017+
char full[CBM_SZ_256];
4018+
if (prop) {
4019+
(void)snprintf(full, sizeof(full), "%s.%s", var, prop);
4020+
} else {
4021+
(void)snprintf(full, sizeof(full), "%s", var);
4022+
}
4023+
for (int i = 0; i < b->var_count; i++) {
4024+
if (strcmp(b->var_names[i], full) == 0) {
4025+
return true;
4026+
}
4027+
}
4028+
return binding_get_edge(b, var) != NULL || binding_get(b, var) != NULL;
4029+
}
4030+
39114031
/* Accumulate aggregation values for a binding */
39124032
static void with_agg_accumulate(with_agg_t *agg, cbm_return_clause_t *wc, binding_t *b) {
39134033
for (int ci = 0; ci < wc->count; ci++) {
39144034
if (!is_aggregate_func(wc->items[ci].func)) {
39154035
continue;
39164036
}
4037+
if (strcmp(wc->items[ci].func, "COUNT") == 0 &&
4038+
!binding_has_value(b, wc->items[ci].variable, wc->items[ci].property)) {
4039+
continue;
4040+
}
39174041
agg->counts[ci]++;
39184042
const char *raw = binding_get_virtual(b, wc->items[ci].variable, wc->items[ci].property);
39194043
if (wc->items[ci].distinct && strcmp(wc->items[ci].func, "COUNT") == 0) {
@@ -4347,6 +4471,10 @@ static void ret_agg_accumulate(ret_agg_entry_t *entry, cbm_return_clause_t *ret,
43474471
if (!is_aggregate_func(ret->items[ci].func)) {
43484472
continue;
43494473
}
4474+
if (strcmp(ret->items[ci].func, "COUNT") == 0 &&
4475+
!binding_has_value(b, ret->items[ci].variable, ret->items[ci].property)) {
4476+
continue;
4477+
}
43504478
entry->counts[ci]++;
43514479
const char *raw = binding_get_virtual(b, ret->items[ci].variable, ret->items[ci].property);
43524480
double dv = strtod(raw, NULL);
@@ -4885,7 +5013,7 @@ static int execute_single(cbm_store_t *store, cbm_query_t *q, const char *projec
48855013
binding_t b = {0};
48865014
b.store = store;
48875015
binding_set(&b, var_name, &scanned[i]);
4888-
bool pass = !q->where || eval_where(q->where, &b);
5016+
bool pass = eval_where_partial(q->where, &b) != CYP_PARTIAL_FALSE;
48895017
if (pass) {
48905018
bindings[bind_count++] = b;
48915019
} else {

‎src/foundation/recursion_whitelist.h‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88
* - parse_or_expr, parse_xor_expr, parse_and_expr, parse_not_expr
99
* - parse_atom_expr, parse_post_where, cbm_parse
1010
*
11-
* Cypher expression evaluator (bounded by WHERE clause depth ~5):
12-
* - eval_expr
11+
* Cypher expression evaluators (bounded by WHERE clause depth ~5):
12+
* - eval_expr, eval_expr_partial
1313
*
1414
* Glob pattern matcher (bounded by pattern nesting ~3):
1515
* - glob_match, glob_match_star, glob_match_doublestar
@@ -25,9 +25,9 @@
2525
* To add a function: add it below AND add NOLINT(misc-no-recursion) on
2626
* the function definition line. The lint gate verifies both match.
2727
*/
28-
#define CBM_RECURSION_WHITELIST \
29-
"parse_or_expr", "parse_xor_expr", "parse_and_expr", "parse_not_expr", "parse_atom_expr", \
30-
"parse_post_where", "cbm_parse", "eval_expr", "glob_match", "glob_match_star", \
31-
"glob_match_doublestar", "glob_match_doublestar_slash", "glob_match_doublestar_any", \
32-
"parse_bool_expr", "parse_bool_atom", "r_collect_imports", \
28+
#define CBM_RECURSION_WHITELIST \
29+
"parse_or_expr", "parse_xor_expr", "parse_and_expr", "parse_not_expr", "parse_atom_expr", \
30+
"parse_post_where", "cbm_parse", "eval_expr", "eval_expr_partial", "glob_match", \
31+
"glob_match_star", "glob_match_doublestar", "glob_match_doublestar_slash", \
32+
"glob_match_doublestar_any", "parse_bool_expr", "parse_bool_atom", "r_collect_imports", \
3333
"find_first_descendant_by_kind", "find_first_descendant_of"

‎tests/test_cypher.c‎

Lines changed: 149 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1673,6 +1673,59 @@ TEST(cypher_exec_count) {
16731673
PASS();
16741674
}
16751675

1676+
TEST(cypher_exec_optional_bound_terminal_count_ignores_unbound) {
1677+
cbm_store_t *s = setup_cypher_store();
1678+
cbm_cypher_result_t r = {0};
1679+
1680+
int rc = cbm_cypher_execute(s,
1681+
"MATCH (f:Function) OPTIONAL MATCH (t)-[:CALLS]->(f) "
1682+
"RETURN f.name, COUNT(t) AS cnt",
1683+
"test", 0, &r);
1684+
ASSERT_EQ(rc, 0);
1685+
ASSERT_EQ(r.row_count, 4);
1686+
1687+
for (int i = 0; i < r.row_count; i++) {
1688+
const char *name = r.rows[i][0];
1689+
const char *count = r.rows[i][1];
1690+
if (strcmp(name, "HandleOrder") == 0) {
1691+
ASSERT_STR_EQ(count, "0");
1692+
} else if (strcmp(name, "ValidateOrder") == 0 || strcmp(name, "SubmitOrder") == 0 ||
1693+
strcmp(name, "LogError") == 0) {
1694+
ASSERT_STR_EQ(count, "1");
1695+
} else {
1696+
ASSERT_TRUE(false);
1697+
}
1698+
}
1699+
1700+
cbm_cypher_result_free(&r);
1701+
cbm_store_close(s);
1702+
PASS();
1703+
}
1704+
1705+
TEST(cypher_exec_with_optional_bound_terminal_count_ignores_unbound) {
1706+
cbm_store_t *s = setup_cypher_store();
1707+
cbm_cypher_result_t r = {0};
1708+
1709+
int rc = cbm_cypher_execute(s,
1710+
"MATCH (f:Function) OPTIONAL MATCH (t)-[:CALLS]->(f) "
1711+
"WITH f, COUNT(t) AS c RETURN f.name, c",
1712+
"test", 0, &r);
1713+
ASSERT_EQ(rc, 0);
1714+
ASSERT_EQ(r.row_count, 4);
1715+
1716+
for (int i = 0; i < r.row_count; i++) {
1717+
if (strcmp(r.rows[i][0], "HandleOrder") == 0) {
1718+
ASSERT_STR_EQ(r.rows[i][1], "0");
1719+
} else {
1720+
ASSERT_STR_EQ(r.rows[i][1], "1");
1721+
}
1722+
}
1723+
1724+
cbm_cypher_result_free(&r);
1725+
cbm_store_close(s);
1726+
PASS();
1727+
}
1728+
16761729
TEST(cypher_exec_limit) {
16771730
cbm_store_t *s = setup_cypher_store();
16781731
cbm_cypher_result_t r = {0};
@@ -2078,9 +2131,10 @@ TEST(cypher_optional_match_target_still_allowed_issue1919) {
20782131
TEST(cypher_with_alias_stays_in_scope_issue1919) {
20792132
cbm_store_t *s = setup_cypher_store();
20802133
cbm_cypher_result_t r = {0};
2081-
int rc = cbm_cypher_execute(
2082-
s, "MATCH (f:Function)-[:CALLS]->(g) WITH f.name AS caller, g AS callee RETURN caller, callee.name",
2083-
"test", 0, &r);
2134+
int rc = cbm_cypher_execute(s,
2135+
"MATCH (f:Function)-[:CALLS]->(g) WITH f.name AS caller, g AS "
2136+
"callee RETURN caller, callee.name",
2137+
"test", 0, &r);
20842138
ASSERT_EQ(rc, 0);
20852139
ASSERT_NULL(r.error);
20862140
ASSERT_TRUE(r.row_count > 0);
@@ -2828,6 +2882,91 @@ TEST(cypher_exec_where_not) {
28282882
PASS();
28292883
}
28302884

2885+
/* Regression (distilled from PR #1245, Andrew Hundt): the early WHERE pass in
2886+
* execute_single runs on seed rows with only the first alias bound. A
2887+
* comparison on an unbound alias used to evaluate to true, which NOT / XOR
2888+
* then inverted -- so every seed was pruned before CALLS expansion and the
2889+
* query returned zero rows. Unbound leaves must stay UNKNOWN there. */
2890+
TEST(cypher_exec_where_not_on_relationship_target) {
2891+
cbm_store_t *s = setup_cypher_store();
2892+
cbm_cypher_result_t r = {0};
2893+
int rc = cbm_cypher_execute(s,
2894+
"MATCH (a:Function)-[:CALLS]->(b:Function) "
2895+
"WHERE NOT b.name CONTAINS \"Order\" RETURN b.name",
2896+
"test", 0, &r);
2897+
ASSERT_EQ(rc, 0);
2898+
ASSERT_EQ(r.row_count, 1);
2899+
ASSERT_STR_EQ(r.rows[0][0], "LogError");
2900+
cbm_cypher_result_free(&r);
2901+
cbm_store_close(s);
2902+
PASS();
2903+
}
2904+
2905+
TEST(cypher_exec_where_mixed_alias_and) {
2906+
cbm_store_t *s = setup_cypher_store();
2907+
cbm_cypher_result_t r = {0};
2908+
int rc = cbm_cypher_execute(
2909+
s,
2910+
"MATCH (a:Function)-[:CALLS]->(b:Function) "
2911+
"WHERE a.name = \"HandleOrder\" AND NOT b.name CONTAINS \"Order\" RETURN b.name",
2912+
"test", 0, &r);
2913+
ASSERT_EQ(rc, 0);
2914+
ASSERT_EQ(r.row_count, 1);
2915+
ASSERT_STR_EQ(r.rows[0][0], "LogError");
2916+
cbm_cypher_result_free(&r);
2917+
cbm_store_close(s);
2918+
PASS();
2919+
}
2920+
2921+
TEST(cypher_exec_where_mixed_alias_or) {
2922+
cbm_store_t *s = setup_cypher_store();
2923+
cbm_cypher_result_t r = {0};
2924+
int rc = cbm_cypher_execute(
2925+
s,
2926+
"MATCH (a:Function)-[:CALLS]->(b:Function) "
2927+
"WHERE a.name = \"NoSuchFunction\" OR NOT b.name CONTAINS \"Order\" RETURN b.name",
2928+
"test", 0, &r);
2929+
ASSERT_EQ(rc, 0);
2930+
ASSERT_EQ(r.row_count, 1);
2931+
ASSERT_STR_EQ(r.rows[0][0], "LogError");
2932+
cbm_cypher_result_free(&r);
2933+
cbm_store_close(s);
2934+
PASS();
2935+
}
2936+
2937+
TEST(cypher_exec_where_mixed_alias_xor) {
2938+
cbm_store_t *s = setup_cypher_store();
2939+
cbm_cypher_result_t r = {0};
2940+
int rc =
2941+
cbm_cypher_execute(s,
2942+
"MATCH (a:Function)-[:CALLS]->(b:Function) "
2943+
"WHERE a.name = \"HandleOrder\" XOR b.name = \"LogError\" RETURN b.name",
2944+
"test", 0, &r);
2945+
ASSERT_EQ(rc, 0);
2946+
ASSERT_EQ(r.row_count, 1);
2947+
ASSERT_STR_EQ(r.rows[0][0], "ValidateOrder");
2948+
cbm_cypher_result_free(&r);
2949+
cbm_store_close(s);
2950+
PASS();
2951+
}
2952+
2953+
/* coalesce(a.nosuch, b.name) has first variable a (bound in the seed) but also
2954+
* needs b; the early pass must treat it as UNKNOWN, not evaluate b as "". */
2955+
TEST(cypher_exec_where_coalesce_multi_alias) {
2956+
cbm_store_t *s = setup_cypher_store();
2957+
cbm_cypher_result_t r = {0};
2958+
int rc = cbm_cypher_execute(s,
2959+
"MATCH (a:Function)-[:CALLS]->(b:Function) "
2960+
"WHERE coalesce(a.nosuch, b.name) = \"LogError\" RETURN b.name",
2961+
"test", 0, &r);
2962+
ASSERT_EQ(rc, 0);
2963+
ASSERT_EQ(r.row_count, 1);
2964+
ASSERT_STR_EQ(r.rows[0][0], "LogError");
2965+
cbm_cypher_result_free(&r);
2966+
cbm_store_close(s);
2967+
PASS();
2968+
}
2969+
28312970
TEST(cypher_exec_where_in) {
28322971
cbm_store_t *s = setup_cypher_store();
28332972
cbm_cypher_result_t r = {0};
@@ -4368,6 +4507,8 @@ SUITE(cypher) {
43684507
RUN_TEST(cypher_exec_calls_with_where);
43694508
RUN_TEST(cypher_exec_inbound);
43704509
RUN_TEST(cypher_exec_count);
4510+
RUN_TEST(cypher_exec_optional_bound_terminal_count_ignores_unbound);
4511+
RUN_TEST(cypher_exec_with_optional_bound_terminal_count_ignores_unbound);
43714512
RUN_TEST(cypher_exec_limit);
43724513
RUN_TEST(cypher_exec_order_by);
43734514
RUN_TEST(cypher_exec_variable_length);
@@ -4418,6 +4559,11 @@ SUITE(cypher) {
44184559
RUN_TEST(cypher_exec_where_neq_bang);
44194560
RUN_TEST(cypher_exec_where_ends_with);
44204561
RUN_TEST(cypher_exec_where_not);
4562+
RUN_TEST(cypher_exec_where_not_on_relationship_target);
4563+
RUN_TEST(cypher_exec_where_mixed_alias_and);
4564+
RUN_TEST(cypher_exec_where_mixed_alias_or);
4565+
RUN_TEST(cypher_exec_where_mixed_alias_xor);
4566+
RUN_TEST(cypher_exec_where_coalesce_multi_alias);
44214567
RUN_TEST(cypher_exec_where_in);
44224568
RUN_TEST(cypher_exec_where_not_in);
44234569
RUN_TEST(cypher_exec_where_is_null);

0 commit comments

Comments
 (0)