Merge pull request #2744 from harshitaajoshi/fix/2591-cypher-where-property-compare

fix(cypher): support property-to-property comparisons in WHERE (#2591)

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 <op> 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;

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 */

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);