Skip to content

Commit 42ca613

Browse files
ngallodevngallodev
committed
fix(cypher): preserve partial where and optional count semantics
1 parent a43b220 commit 42ca613

3 files changed

Lines changed: 226 additions & 11 deletions

File tree

‎src/cypher/cypher.c‎

Lines changed: 113 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2750,6 +2750,96 @@ 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+
return eval_condition(c, b) ? CYP_PARTIAL_TRUE : CYP_PARTIAL_FALSE;
2785+
}
2786+
2787+
static cyp_partial_t eval_expr_partial(const cbm_expr_t *e, // NOLINT(misc-no-recursion)
2788+
binding_t *b) {
2789+
if (!e) {
2790+
return CYP_PARTIAL_TRUE;
2791+
}
2792+
if (e->type == EXPR_CONDITION) {
2793+
return eval_condition_partial(&e->cond, b);
2794+
}
2795+
cyp_partial_t left = eval_expr_partial(e->left, b);
2796+
if (e->type == EXPR_NOT) {
2797+
if (left == CYP_PARTIAL_UNKNOWN) {
2798+
return CYP_PARTIAL_UNKNOWN;
2799+
}
2800+
return left == CYP_PARTIAL_TRUE ? CYP_PARTIAL_FALSE : CYP_PARTIAL_TRUE;
2801+
}
2802+
/* Same short-circuits as eval_expr: a definite left decides AND / OR. */
2803+
if (e->type == EXPR_AND && left == CYP_PARTIAL_FALSE) {
2804+
return CYP_PARTIAL_FALSE;
2805+
}
2806+
if (e->type == EXPR_OR && left == CYP_PARTIAL_TRUE) {
2807+
return CYP_PARTIAL_TRUE;
2808+
}
2809+
cyp_partial_t right = eval_expr_partial(e->right, b);
2810+
if (e->type == EXPR_AND) {
2811+
return partial_and(left, right);
2812+
}
2813+
if (e->type == EXPR_OR) {
2814+
return partial_or(left, right);
2815+
}
2816+
/* EXPR_XOR */
2817+
if (left == CYP_PARTIAL_UNKNOWN || right == CYP_PARTIAL_UNKNOWN) {
2818+
return CYP_PARTIAL_UNKNOWN;
2819+
}
2820+
return left != right ? CYP_PARTIAL_TRUE : CYP_PARTIAL_FALSE;
2821+
}
2822+
2823+
static cyp_partial_t eval_where_partial(const cbm_where_clause_t *w, binding_t *b) {
2824+
if (!w) {
2825+
return CYP_PARTIAL_TRUE;
2826+
}
2827+
if (w->root) {
2828+
return eval_expr_partial(w->root, b);
2829+
}
2830+
/* Legacy flat evaluation */
2831+
if (w->count == 0) {
2832+
return CYP_PARTIAL_TRUE;
2833+
}
2834+
bool is_and = (w->op && strcmp(w->op, "AND") == 0) != 0;
2835+
cyp_partial_t result = is_and ? CYP_PARTIAL_TRUE : CYP_PARTIAL_FALSE;
2836+
for (int i = 0; i < w->count; i++) {
2837+
cyp_partial_t r = eval_condition_partial(&w->conditions[i], b);
2838+
result = is_and ? partial_and(result, r) : partial_or(result, r);
2839+
}
2840+
return result;
2841+
}
2842+
27532843
/* Check if a string value looks like a regex pattern. */
27542844
static bool looks_like_regex(const char *s) {
27552845
if (!s) {
@@ -4342,11 +4432,33 @@ static void ret_agg_init_group(ret_agg_entry_t *entry, const char *key, int item
43424432
}
43434433

43444434
/* Accumulate a binding into RETURN aggregation */
4435+
static bool binding_has_value(binding_t *b, const char *var, const char *prop) {
4436+
if (!var || strcmp(var, "*") == 0) {
4437+
return true;
4438+
}
4439+
char full[CBM_SZ_256];
4440+
if (prop) {
4441+
snprintf(full, sizeof(full), "%s.%s", var, prop);
4442+
} else {
4443+
snprintf(full, sizeof(full), "%s", var);
4444+
}
4445+
for (int i = 0; i < b->var_count; i++) {
4446+
if (strcmp(b->var_names[i], full) == 0) {
4447+
return true;
4448+
}
4449+
}
4450+
return binding_get_edge(b, var) != NULL || binding_get(b, var) != NULL;
4451+
}
4452+
43454453
static void ret_agg_accumulate(ret_agg_entry_t *entry, cbm_return_clause_t *ret, binding_t *b) {
43464454
for (int ci = 0; ci < ret->count; ci++) {
43474455
if (!is_aggregate_func(ret->items[ci].func)) {
43484456
continue;
43494457
}
4458+
if (strcmp(ret->items[ci].func, "COUNT") == 0 &&
4459+
!binding_has_value(b, ret->items[ci].variable, ret->items[ci].property)) {
4460+
continue;
4461+
}
43504462
entry->counts[ci]++;
43514463
const char *raw = binding_get_virtual(b, ret->items[ci].variable, ret->items[ci].property);
43524464
double dv = strtod(raw, NULL);
@@ -4885,7 +4997,7 @@ static int execute_single(cbm_store_t *store, cbm_query_t *q, const char *projec
48854997
binding_t b = {0};
48864998
b.store = store;
48874999
binding_set(&b, var_name, &scanned[i]);
4888-
bool pass = !q->where || eval_where(q->where, &b);
5000+
bool pass = eval_where_partial(q->where, &b) != CYP_PARTIAL_FALSE;
48895001
if (pass) {
48905002
bindings[bind_count++] = b;
48915003
} 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: 106 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1673,6 +1673,35 @@ 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+
16761705
TEST(cypher_exec_limit) {
16771706
cbm_store_t *s = setup_cypher_store();
16781707
cbm_cypher_result_t r = {0};
@@ -2078,9 +2107,10 @@ TEST(cypher_optional_match_target_still_allowed_issue1919) {
20782107
TEST(cypher_with_alias_stays_in_scope_issue1919) {
20792108
cbm_store_t *s = setup_cypher_store();
20802109
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);
2110+
int rc = cbm_cypher_execute(s,
2111+
"MATCH (f:Function)-[:CALLS]->(g) WITH f.name AS caller, g AS "
2112+
"callee RETURN caller, callee.name",
2113+
"test", 0, &r);
20842114
ASSERT_EQ(rc, 0);
20852115
ASSERT_NULL(r.error);
20862116
ASSERT_TRUE(r.row_count > 0);
@@ -2828,6 +2858,74 @@ TEST(cypher_exec_where_not) {
28282858
PASS();
28292859
}
28302860

2861+
/* Regression (distilled from PR #1245, Andrew Hundt): the early WHERE pass in
2862+
* execute_single runs on seed rows with only the first alias bound. A
2863+
* comparison on an unbound alias used to evaluate to true, which NOT / XOR
2864+
* then inverted -- so every seed was pruned before CALLS expansion and the
2865+
* query returned zero rows. Unbound leaves must stay UNKNOWN there. */
2866+
TEST(cypher_exec_where_not_on_relationship_target) {
2867+
cbm_store_t *s = setup_cypher_store();
2868+
cbm_cypher_result_t r = {0};
2869+
int rc = cbm_cypher_execute(s,
2870+
"MATCH (a:Function)-[:CALLS]->(b:Function) "
2871+
"WHERE NOT b.name CONTAINS \"Order\" RETURN b.name",
2872+
"test", 0, &r);
2873+
ASSERT_EQ(rc, 0);
2874+
ASSERT_EQ(r.row_count, 1);
2875+
ASSERT_STR_EQ(r.rows[0][0], "LogError");
2876+
cbm_cypher_result_free(&r);
2877+
cbm_store_close(s);
2878+
PASS();
2879+
}
2880+
2881+
TEST(cypher_exec_where_mixed_alias_and) {
2882+
cbm_store_t *s = setup_cypher_store();
2883+
cbm_cypher_result_t r = {0};
2884+
int rc = cbm_cypher_execute(
2885+
s,
2886+
"MATCH (a:Function)-[:CALLS]->(b:Function) "
2887+
"WHERE a.name = \"HandleOrder\" AND NOT b.name CONTAINS \"Order\" RETURN b.name",
2888+
"test", 0, &r);
2889+
ASSERT_EQ(rc, 0);
2890+
ASSERT_EQ(r.row_count, 1);
2891+
ASSERT_STR_EQ(r.rows[0][0], "LogError");
2892+
cbm_cypher_result_free(&r);
2893+
cbm_store_close(s);
2894+
PASS();
2895+
}
2896+
2897+
TEST(cypher_exec_where_mixed_alias_or) {
2898+
cbm_store_t *s = setup_cypher_store();
2899+
cbm_cypher_result_t r = {0};
2900+
int rc = cbm_cypher_execute(
2901+
s,
2902+
"MATCH (a:Function)-[:CALLS]->(b:Function) "
2903+
"WHERE a.name = \"NoSuchFunction\" OR NOT b.name CONTAINS \"Order\" RETURN b.name",
2904+
"test", 0, &r);
2905+
ASSERT_EQ(rc, 0);
2906+
ASSERT_EQ(r.row_count, 1);
2907+
ASSERT_STR_EQ(r.rows[0][0], "LogError");
2908+
cbm_cypher_result_free(&r);
2909+
cbm_store_close(s);
2910+
PASS();
2911+
}
2912+
2913+
TEST(cypher_exec_where_mixed_alias_xor) {
2914+
cbm_store_t *s = setup_cypher_store();
2915+
cbm_cypher_result_t r = {0};
2916+
int rc =
2917+
cbm_cypher_execute(s,
2918+
"MATCH (a:Function)-[:CALLS]->(b:Function) "
2919+
"WHERE a.name = \"HandleOrder\" XOR b.name = \"LogError\" RETURN b.name",
2920+
"test", 0, &r);
2921+
ASSERT_EQ(rc, 0);
2922+
ASSERT_EQ(r.row_count, 1);
2923+
ASSERT_STR_EQ(r.rows[0][0], "ValidateOrder");
2924+
cbm_cypher_result_free(&r);
2925+
cbm_store_close(s);
2926+
PASS();
2927+
}
2928+
28312929
TEST(cypher_exec_where_in) {
28322930
cbm_store_t *s = setup_cypher_store();
28332931
cbm_cypher_result_t r = {0};
@@ -4368,6 +4466,7 @@ SUITE(cypher) {
43684466
RUN_TEST(cypher_exec_calls_with_where);
43694467
RUN_TEST(cypher_exec_inbound);
43704468
RUN_TEST(cypher_exec_count);
4469+
RUN_TEST(cypher_exec_optional_bound_terminal_count_ignores_unbound);
43714470
RUN_TEST(cypher_exec_limit);
43724471
RUN_TEST(cypher_exec_order_by);
43734472
RUN_TEST(cypher_exec_variable_length);
@@ -4418,6 +4517,10 @@ SUITE(cypher) {
44184517
RUN_TEST(cypher_exec_where_neq_bang);
44194518
RUN_TEST(cypher_exec_where_ends_with);
44204519
RUN_TEST(cypher_exec_where_not);
4520+
RUN_TEST(cypher_exec_where_not_on_relationship_target);
4521+
RUN_TEST(cypher_exec_where_mixed_alias_and);
4522+
RUN_TEST(cypher_exec_where_mixed_alias_or);
4523+
RUN_TEST(cypher_exec_where_mixed_alias_xor);
44214524
RUN_TEST(cypher_exec_where_in);
44224525
RUN_TEST(cypher_exec_where_not_in);
44234526
RUN_TEST(cypher_exec_where_is_null);

0 commit comments

Comments
 (0)