diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index fe11f06fb..17ba3df38 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -791,6 +791,8 @@ static void expr_free(cbm_expr_t *e) { safe_str_free(&cur->cond.op); safe_str_free(&cur->cond.value); safe_str_free(&cur->cond.coalesce_default); + safe_str_free(&cur->cond.rhs_variable); + safe_str_free(&cur->cond.rhs_property); for (int i = 0; i < cur->cond.in_value_count; i++) { safe_str_free(&cur->cond.in_values[i]); } @@ -1081,9 +1083,49 @@ static cbm_expr_t *parse_exists_predicate(parser_t *p, bool negated) { return expr_leaf(c); } +/* Free what a condition parser owns when it gives up mid-condition. */ +static void cond_parse_fail_free(cbm_condition_t *c) { + cond_func_fields_free(c); + safe_str_free(&c->variable); + safe_str_free(&c->property); + safe_str_free(&c->op); + safe_str_free(&c->coalesce_default); + safe_str_free(&c->rhs_variable); + safe_str_free(&c->rhs_property); +} + +enum { COND_RHS_REF = 1 }; + +/* RHS operand that names a row value instead of a literal: var.prop, or a bare + * alias such as a WITH projection (#2591). Returns COND_RHS_REF when one was + * parsed, 0 when the next token is not one, CBM_NOT_FOUND with p->error set + * when it is malformed or a function call. */ +static int parse_condition_rhs_ref(parser_t *p, cbm_condition_t *c) { + if (p->pos + SKIP_ONE < p->count && p->tokens[p->pos + SKIP_ONE].type == TOK_LPAREN) { + snprintf(p->error, sizeof(p->error), + "unsupported function '%s' on the right side of a WHERE comparison " + "(only literals and var.prop are supported)", + peek(p)->text); + return CBM_NOT_FOUND; + } + if (!check(p, TOK_IDENT)) { + return 0; + } + c->rhs_variable = heap_strdup(advance(p)->text); + if (match(p, TOK_DOT)) { + const cbm_token_t *prop = expect(p, TOK_IDENT); + if (!prop) { + return CBM_NOT_FOUND; + } + c->rhs_property = heap_strdup(prop->text); + } + return COND_RHS_REF; +} + /* Parse the operator + value tail shared by every condition subject * (var[.prop] and multi-arg functions like coalesce(...)): IS [NOT] NULL, - * IN [...], or a comparison operator with a literal value. */ + * IN [...], or a comparison operator with a literal value or a property + * reference (a.x = b.y). */ static cbm_expr_t *parse_condition_op(parser_t *p, cbm_condition_t *c) { /* IS NULL / IS NOT NULL */ if (check(p, TOK_IS)) { @@ -1111,10 +1153,7 @@ static cbm_expr_t *parse_condition_op(parser_t *p, cbm_condition_t *c) { c->op = parse_comparison_op(p); if (!c->op) { snprintf(p->error, sizeof(p->error), "unexpected operator at pos %d", peek(p)->pos); - cond_func_fields_free(c); - safe_str_free(&c->variable); - safe_str_free(&c->property); - safe_str_free(&c->coalesce_default); + cond_parse_fail_free(c); return NULL; } @@ -1127,13 +1166,11 @@ static cbm_expr_t *parse_condition_op(parser_t *p, cbm_condition_t *c) { } else if (check(p, TOK_FALSE)) { advance(p); c->value = heap_strdup("false"); - } else { - snprintf(p->error, sizeof(p->error), "expected value at pos %d", peek(p)->pos); - cond_func_fields_free(c); - safe_str_free(&c->variable); - safe_str_free(&c->property); - safe_str_free(&c->op); - safe_str_free(&c->coalesce_default); + } else if (parse_condition_rhs_ref(p, c) <= 0) { + if (!p->error[0]) { + snprintf(p->error, sizeof(p->error), "expected value at pos %d", peek(p)->pos); + } + cond_parse_fail_free(c); return NULL; } @@ -2227,6 +2264,8 @@ static void free_where(cbm_where_clause_t *w) { free(w->conditions[i].in_values); safe_str_free(&w->conditions[i].func); func_args_free(w->conditions[i].args, w->conditions[i].arg_count); + safe_str_free(&w->conditions[i].rhs_variable); + safe_str_free(&w->conditions[i].rhs_property); } free(w->conditions); safe_str_free(&w->op); @@ -2732,6 +2771,59 @@ static bool eval_comparison_op(const char *op, const char *actual, const char *e return false; } +enum { EXISTS_DIR_OUT = 0, EXISTS_DIR_IN = 1 }; + +/* EXISTS { (var)-[:TYPE]->() }: does the bound node have any edge of the + * given type in the requested direction? (dir 0=out, 1=in, 2=any) */ +static bool eval_exists_condition(const cbm_condition_t *c, binding_t *b) { + cbm_node_t *n = binding_get(b, c->variable); + bool result = false; + if (n && b->store) { + cbm_edge_t *edges = NULL; + int cnt = 0; + if (c->exists_dir != EXISTS_DIR_IN) { /* outbound or any */ + if (c->value) { + cbm_store_find_edges_by_source_type(b->store, n->id, c->value, &edges, &cnt); + } else { + cbm_store_find_edges_by_source(b->store, n->id, &edges, &cnt); + } + result = cnt > 0; + cbm_store_free_edges(edges, cnt); + } + if (!result && c->exists_dir != EXISTS_DIR_OUT) { /* inbound or any */ + edges = NULL; + cnt = 0; + if (c->value) { + cbm_store_find_edges_by_target_type(b->store, n->id, c->value, &edges, &cnt); + } else { + cbm_store_find_edges_by_target(b->store, n->id, &edges, &cnt); + } + result = cnt > 0; + cbm_store_free_edges(edges, cnt); + } + } + return c->negated ? !result : result; +} + +/* a.x b.y (#2591): the RHS operand is read from the same row. An unbound + * RHS alias does not filter, like an unbound LHS. A missing (empty) property + * on either side is NULL and the comparison is false, so two nodes that both + * lack a property do not compare equal. */ +static bool eval_property_comparison(const cbm_condition_t *c, const char *actual, binding_t *b) { + cbm_condition_t rhs = {0}; + rhs.variable = c->rhs_variable; + rhs.property = c->rhs_property; + const char *expected = resolve_condition_value(&rhs, b); + if (!expected) { + return true; + } + if (actual[0] == '\0' || expected[0] == '\0') { + return false; + } + bool result = eval_comparison_op(c->op, actual, expected); + return c->negated ? !result : result; +} + /* Evaluate a WHERE condition against a binding */ static bool eval_condition(const cbm_condition_t *c, binding_t *b) { /* Label test: WHERE n:Label (#241) — compare the bound node's label @@ -2742,36 +2834,8 @@ static bool eval_condition(const cbm_condition_t *c, binding_t *b) { return c->negated ? !result : result; } - /* EXISTS { (var)-[:TYPE]->() }: does the bound node have any edge of the - * given type in the requested direction? (dir 0=out, 1=in, 2=any) */ if (strcmp(c->op, "EXISTS") == 0) { - cbm_node_t *n = binding_get(b, c->variable); - bool result = false; - if (n && b->store) { - cbm_edge_t *edges = NULL; - int cnt = 0; - if (c->exists_dir != 1) { /* outbound or any */ - if (c->value) { - cbm_store_find_edges_by_source_type(b->store, n->id, c->value, &edges, &cnt); - } else { - cbm_store_find_edges_by_source(b->store, n->id, &edges, &cnt); - } - result = cnt > 0; - cbm_store_free_edges(edges, cnt); - } - if (!result && c->exists_dir != 0) { /* inbound or any */ - edges = NULL; - cnt = 0; - if (c->value) { - cbm_store_find_edges_by_target_type(b->store, n->id, c->value, &edges, &cnt); - } else { - cbm_store_find_edges_by_target(b->store, n->id, &edges, &cnt); - } - result = cnt > 0; - cbm_store_free_edges(edges, cnt); - } - } - return c->negated ? !result : result; + return eval_exists_condition(c, b); } const char *actual = resolve_condition_value(c, b); @@ -2808,6 +2872,9 @@ static bool eval_condition(const cbm_condition_t *c, binding_t *b) { return c->negated ? !result : result; } + if (c->rhs_variable) { + return eval_property_comparison(c, actual, b); + } result = eval_comparison_op(c->op, actual, c->value); return c->negated ? !result : result; } @@ -2904,11 +2971,12 @@ static bool partial_alias_unbound(binding_t *b, const char *alias) { } /* A condition is UNKNOWN while any alias it reads is not bound yet: its own - * variable, and every function argument's -- a func condition is filed under - * its first argument's alias, so the b in coalesce(a.x, b.name) is only seen - * here. A literal-only LHS has nothing to wait for. */ + * variable, the RHS of a property comparison (the b in a.x = b.y), and every + * function argument's -- a func condition is filed under its first argument's + * alias, so the b in coalesce(a.x, b.name) is only seen here. A literal-only + * LHS has nothing to wait for. */ static cyp_partial_t eval_condition_partial(const cbm_condition_t *c, binding_t *b) { - if (partial_alias_unbound(b, c->variable)) { + if (partial_alias_unbound(b, c->variable) || partial_alias_unbound(b, c->rhs_variable)) { return CYP_PARTIAL_UNKNOWN; } for (int i = 0; i < c->arg_count; i++) { @@ -5918,7 +5986,8 @@ static int cypher_cond_selectivity(const cbm_condition_t *c, const char *var) { if (!c || !c->variable || !var || strcmp(c->variable, var) != 0 || c->negated) { return 0; } - if (!c->op || strcmp(c->op, "=") != 0 || !c->property) { + /* a.x = b.y pins no value on either alias, so it seeds nothing. */ + if (!c->op || strcmp(c->op, "=") != 0 || !c->property || c->rhs_variable) { return 0; } return (strcmp(c->property, "name") == 0 || strcmp(c->property, "qualified_name") == 0) ? 3 : 2; diff --git a/src/cypher/cypher.h b/src/cypher/cypher.h index 10ef0aeec..855dcb06c 100644 --- a/src/cypher/cypher.h +++ b/src/cypher/cypher.h @@ -210,6 +210,11 @@ typedef struct { const char *func; cbm_func_arg_t *args; int arg_count; + /* Property reference on the RHS, e.g. a.file_path = b.file_path (#2591). + * When rhs_variable is set the comparison reads its operand from the row + * instead of `value`; rhs_property NULL = bare alias. */ + const char *rhs_variable; + const char *rhs_property; } cbm_condition_t; /* Expression tree for WHERE clause */ diff --git a/tests/test_cypher.c b/tests/test_cypher.c index b3b28b633..7cea25507 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -3484,6 +3484,160 @@ TEST(cypher_exec_where_func_arg_on_unbound_target) { PASS(); } +/* #2591: a WHERE comparison whose RHS is another property (a.file_path = + * b.file_path) failed to parse with "expected value at pos N". Fixture: Caller + * calls Helper in the same file and Remote in another one. */ +static cbm_store_t *setup_issue2591_store(void) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + cbm_node_t caller = {.project = "test", + .label = "Function", + .name = "Caller", + .qualified_name = "test.Caller", + .file_path = "a.go", + .start_line = 1, + .end_line = 3}; + cbm_node_t helper = {.project = "test", + .label = "Function", + .name = "Helper", + .qualified_name = "test.Helper", + .file_path = "a.go", + .start_line = 5, + .end_line = 7}; + cbm_node_t remote = {.project = "test", + .label = "Function", + .name = "Remote", + .qualified_name = "test.Remote", + .file_path = "b.go", + .start_line = 1, + .end_line = 2}; + int64_t id_caller = cbm_store_upsert_node(s, &caller); + int64_t id_helper = cbm_store_upsert_node(s, &helper); + int64_t id_remote = cbm_store_upsert_node(s, &remote); + cbm_edge_t e1 = { + .project = "test", .source_id = id_caller, .target_id = id_helper, .type = "CALLS"}; + cbm_edge_t e2 = { + .project = "test", .source_id = id_caller, .target_id = id_remote, .type = "CALLS"}; + cbm_store_insert_edge(s, &e1); + cbm_store_insert_edge(s, &e2); + return s; +} + +/* Run `query` on the #2591 fixture and expect exactly one row whose last + * column is `want`. */ +static int issue2591_expect_one(const char *query, const char *want) { + cbm_store_t *s = setup_issue2591_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, query, "test", 0, &r); + int ok = rc == 0 && r.row_count == 1 && r.col_count > 0 && + strcmp(r.rows[0][r.col_count - 1], want) == 0; + if (!ok) { + printf(" query: %s\n rc=%d rows=%d err=%s\n", query, rc, r.row_count, + r.error ? r.error : "(none)"); + } + cbm_cypher_result_free(&r); + cbm_store_close(s); + return ok; +} + +TEST(cypher_issue2591_parse_where_property_rhs) { + cbm_query_t *q = NULL; + char *err = NULL; + int rc = cbm_cypher_parse("MATCH (a)-[:CALLS]->(b) WHERE a.file_path <> b.file_path RETURN a", + &q, &err); + ASSERT_EQ(rc, 0); + ASSERT_NOT_NULL(q->where); + ASSERT_NOT_NULL(q->where->root); + ASSERT_EQ(q->where->root->type, EXPR_CONDITION); + ASSERT_STR_EQ(q->where->root->cond.variable, "a"); + ASSERT_STR_EQ(q->where->root->cond.property, "file_path"); + ASSERT_STR_EQ(q->where->root->cond.op, "<>"); + ASSERT_STR_EQ(q->where->root->cond.rhs_variable, "b"); + ASSERT_STR_EQ(q->where->root->cond.rhs_property, "file_path"); + ASSERT_NULL(q->where->root->cond.value); + cbm_query_free(q); + PASS(); +} + +TEST(cypher_issue2591_where_property_eq_and_neq) { + ASSERT_TRUE(issue2591_expect_one("MATCH (a)-[:CALLS]->(b) WHERE a.file_path = b.file_path " + "RETURN a.name, b.name", + "Helper")); + ASSERT_TRUE(issue2591_expect_one("MATCH (a)-[:CALLS]->(b) WHERE a.file_path <> b.file_path " + "RETURN a.name, b.name", + "Remote")); + /* Operand order does not matter, and the RHS can sit inside an AND chain + * next to a literal comparison. */ + ASSERT_TRUE(issue2591_expect_one("MATCH (a)-[:CALLS]->(b) WHERE b.file_path = a.file_path " + "AND a.name = \"Caller\" RETURN b.name", + "Helper")); + PASS(); +} + +TEST(cypher_issue2591_where_property_after_with) { + ASSERT_TRUE(issue2591_expect_one("MATCH (a)-[:CALLS]->(b) WITH a, b " + "WHERE a.file_path = b.file_path RETURN b.name", + "Helper")); + /* Bare projected aliases on both sides. */ + ASSERT_TRUE(issue2591_expect_one("MATCH (a)-[:CALLS]->(b) WITH a.file_path AS af, " + "b.file_path AS bf, b.name AS callee " + "WHERE af <> bf RETURN callee", + "Remote")); + PASS(); +} + +/* The early WHERE pass binds only the seed alias a. The RHS b is not bound + * there, so the leaf must be UNKNOWN; reading it as a match would let the + * outer NOT prune every seed before CALLS expansion. */ +TEST(cypher_issue2591_where_property_rhs_unbound_in_early_pass) { + ASSERT_TRUE(issue2591_expect_one( + "MATCH (a)-[:CALLS]->(b) WHERE NOT (a.file_path = b.file_path) RETURN b.name", "Remote")); + ASSERT_TRUE(issue2591_expect_one( + "MATCH (a)-[:CALLS]->(b) WHERE NOT a.file_path = b.file_path RETURN b.name", "Remote")); + PASS(); +} + +TEST(cypher_issue2591_where_property_numeric) { + /* Caller starts on line 1, Helper on 5, Remote on 1. */ + ASSERT_TRUE(issue2591_expect_one( + "MATCH (a)-[:CALLS]->(b) WHERE a.start_line < b.start_line RETURN b.name", "Helper")); + ASSERT_TRUE(issue2591_expect_one( + "MATCH (a)-[:CALLS]->(b) WHERE a.start_line >= b.start_line RETURN b.name", "Remote")); + PASS(); +} + +/* A missing property is NULL, and a comparison with NULL never matches: two + * absent properties must not compare equal. */ +TEST(cypher_issue2591_where_property_missing_is_null) { + const char *queries[] = { + "MATCH (a)-[:CALLS]->(b) WHERE a.no_such_prop = b.no_such_prop RETURN b.name", + "MATCH (a)-[:CALLS]->(b) WHERE a.no_such_prop <> b.file_path RETURN b.name", + }; + for (size_t i = 0; i < sizeof(queries) / sizeof(queries[0]); i++) { + cbm_store_t *s = setup_issue2591_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, queries[i], "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 0); + cbm_cypher_result_free(&r); + cbm_store_close(s); + } + PASS(); +} + +TEST(cypher_issue2591_where_function_rhs_unsupported) { + cbm_query_t *q = NULL; + char *err = NULL; + const char *query = "MATCH (a)-[:CALLS]->(b) WHERE a.name = toLower(b.name) RETURN a"; + int rc = cbm_cypher_parse(query, &q, &err); + ASSERT_EQ(rc, -1); + ASSERT_NOT_NULL(err); + ASSERT_TRUE(strstr(err, "unsupported function 'toLower' on the right side of a WHERE " + "comparison") != NULL); + free(err); + PASS(); +} + TEST(cypher_exec_where_in) { cbm_store_t *s = setup_cypher_store(); cbm_cypher_result_t r = {0}; @@ -6456,6 +6610,13 @@ SUITE(cypher) { RUN_TEST(cypher_exec_where_not_source_node); RUN_TEST(cypher_exec_where_not_target_node_count); RUN_TEST(cypher_exec_where_func_arg_on_unbound_target); + RUN_TEST(cypher_issue2591_parse_where_property_rhs); + RUN_TEST(cypher_issue2591_where_property_eq_and_neq); + RUN_TEST(cypher_issue2591_where_property_after_with); + RUN_TEST(cypher_issue2591_where_property_rhs_unbound_in_early_pass); + RUN_TEST(cypher_issue2591_where_property_numeric); + RUN_TEST(cypher_issue2591_where_property_missing_is_null); + RUN_TEST(cypher_issue2591_where_function_rhs_unsupported); RUN_TEST(cypher_exec_where_in); RUN_TEST(cypher_exec_where_not_in); RUN_TEST(cypher_exec_where_is_null);