Push single-variable WHERE predicates across all MATCH clauses

src/executor.rs

@@ -1,3 +1,4 @@
+use std::cell::Cell;
 use std::cell::RefCell;
 use std::collections::HashMap;
 use std::collections::HashSet;
@@ -109,6 +110,7 @@ pub fn execute<G: GraphProvider>(graph: &G, query: &Query) -> Result<ResultSet,
         reverse_cache: RefCell::new(HashMap::new()),
         in_hoist: RefCell::new(HashMap::new()),
         pushdown: RefCell::new(HashMap::new()),
+        pushdown_active: Cell::new(false),
     };
     executor.run(query)
 }
@@ -123,17 +125,29 @@ struct Executor<'a, G: GraphProvider> {
     /// expression's address, so `x IN <constant list>` is an O(1) `HashSet` lookup instead of an
     /// O(list) scan re-evaluated per row. Populated for the duration of a single `filter_rows` pass.
     in_hoist: RefCell<HashMap<usize, Rc<HashSet<CypherValue>>>>,
-    /// Single-variable WHERE predicates pushed down into candidate generation, keyed by variable
-    /// name, so scans prune before pattern expansion. Set for the duration of a MATCH clause's
-    /// pattern expansion; the full WHERE is still applied afterwards.
+    /// Single-variable WHERE predicates from every non-optional MATCH clause, keyed by the variable
+    /// they reference, so a predicate prunes the scan/expansion that first binds its variable — even
+    /// when written on a later clause. Built once per query in `run`; the full WHERE is still applied
+    /// afterwards, so this is a safe pre-filter.
     pushdown: RefCell<HashMap<String, Vec<Expr>>>,
+    /// Gates pushdown application to non-optional MATCH pattern evaluation only. It must stay off
+    /// during `OPTIONAL MATCH` expansion and `EXISTS` subqueries: there, pruning a candidate that
+    /// fails a single-variable predicate can change results (e.g. an `OPTIONAL MATCH ... WHERE t IS
+    /// NULL` anti-join would null-fill every row and wrongly keep them all).
+    pushdown_active: Cell<bool>,
 }
 
 impl<G: GraphProvider> Executor<'_, G> {
     fn run(&self, query: &Query) -> Result<ResultSet, CypherError> {
         let mut rows: Vec<Row<G::NodeId>> = vec![Row::new()];
         let mut scope: Vec<String> = Vec::new();
 
+        // Build the pushdown map once from every non-optional MATCH's WHERE, so a single-variable
+        // predicate prunes the scan/expansion that first binds its variable even when the WHERE is
+        // written on a later clause. Without this, `MATCH (t)-[:R]->(c) MATCH (c)-..-(x) WHERE
+        // t.name = ...` materializes the entire `(t)-[:R]->(c)` relation before the `t` filter runs.
+        *self.pushdown.borrow_mut() = build_global_pushdown(&query.clauses);
+
         for clause in &query.clauses {
             match clause {
                 Clause::Match(m) if m.optional => {
@@ -178,14 +192,16 @@ impl<G: GraphProvider> Executor<'_, G> {
             HashSet::new()
         };
 
-        // Push single-variable predicates down into candidate generation so scans prune before
-        // expansion. The full WHERE still runs afterwards, so this is a safe pre-filter.
-        *self.pushdown.borrow_mut() = build_pushdown(&clause.where_clause);
+        // Apply the (global) pushdown map only while evaluating this non-optional MATCH's patterns:
+        // scans prune before expansion, and the full WHERE still runs afterwards. Pushdown must stay
+        // off during `OPTIONAL MATCH` expansion and `EXISTS` (see `pushdown_active`), so it is gated
+        // rather than always-on.
+        self.pushdown_active.set(true);
         for pattern in &clause.patterns {
             rows = self.eval_pattern(rows, pattern)?;
             add_pattern_vars(scope, pattern);
         }
-        self.pushdown.borrow_mut().clear();
+        self.pushdown_active.set(false);
 
         if let Some(predicate) = &clause.where_clause {
             rows = self.filter_rows_hoisted(rows, predicate, &constant_vars)?;
@@ -197,6 +213,9 @@ impl<G: GraphProvider> Executor<'_, G> {
     /// variable. Best-effort: evaluation errors are ignored here (the full WHERE runs later and
     /// will surface them), so a node is only pruned when a pushed predicate is definitively false.
     fn node_passes_pushdown(&self, var: &Option<String>, node: G::NodeId) -> bool {
+        if !self.pushdown_active.get() {
+            return true;
+        }
         let Some(name) = var else {
             return true;
         };
@@ -1128,16 +1147,25 @@ fn resolve_order_column(
     )))
 }
 
-/// Builds the pushdown map: single-variable, aggregate/EXISTS-free `WHERE` conjuncts keyed by the
-/// variable they reference.
-fn build_pushdown(where_clause: &Option<Expr>) -> HashMap<String, Vec<Expr>> {
+/// Builds the pushdown map from every non-optional MATCH clause's `WHERE`: single-variable,
+/// aggregate/EXISTS-free conjuncts keyed by the variable they reference.
+///
+/// Only non-optional MATCH predicates are safe to push into candidate generation: they are inner
+/// selections, so pruning a candidate that fails a single-variable predicate can never drop a row
+/// the final WHERE would have kept. `OPTIONAL MATCH` (outer join) and `WITH ... WHERE` are skipped.
+fn build_global_pushdown(clauses: &[Clause]) -> HashMap<String, Vec<Expr>> {
     let mut map: HashMap<String, Vec<Expr>> = HashMap::new();
-    if let Some(predicate) = where_clause {
-        let mut conjuncts = Vec::new();
-        collect_conjuncts(predicate, &mut conjuncts);
-        for conjunct in conjuncts {
-            if let Some(var) = single_var(conjunct) {
-                map.entry(var).or_default().push(conjunct.clone());
+    for clause in clauses {
+        if let Clause::Match(m) = clause
+            && !m.optional
+            && let Some(predicate) = &m.where_clause
+        {
+            let mut conjuncts = Vec::new();
+            collect_conjuncts(predicate, &mut conjuncts);
+            for conjunct in conjuncts {
+                if let Some(var) = single_var(conjunct) {
+                    map.entry(var).or_default().push(conjunct.clone());
+                }
             }
         }
     }

tests/cross_clause_pushdown.rs

@@ -0,0 +1,139 @@
+//! Tests that a single-variable `WHERE` predicate is pushed into candidate generation even when it
+//! is written on a *later* MATCH clause than the one that first binds its variable.
+//!
+//! Without cross-clause pushdown, `MATCH (t)-[:R]->(c) MATCH (c)-[:S]->(x) WHERE t.name = '...'`
+//! materializes the entire `(t)-[:R]->(c)` relation before the `t` filter runs. A spy provider
+//! counts how many nodes the first clause expands from, so we can assert that only the matching
+//! `t` is expanded, not every candidate.
+
+use std::cell::RefCell;
+use std::collections::HashMap;
+
+use cypher_parser::{CypherValue, GraphProvider, execute, parse};
+
+/// A tiny graph: several `Class` nodes, each with one child `Class` via `HAS_CHILD`, and each child
+/// owning one `Thing` via `HAS_THING`. Only the "Target" class's subtree should survive the query.
+struct SpyGraph {
+    labels: Vec<Vec<String>>,
+    names: Vec<String>,
+    forward: HashMap<(usize, String), Vec<usize>>,
+    /// Nodes we were asked to expand along `HAS_CHILD`, in order — the signal that the `t` scan
+    /// was (or wasn't) pruned before expansion.
+    has_child_expansions: RefCell<Vec<usize>>,
+}
+
+impl SpyGraph {
+    fn new() -> Self {
+        // Classes 0..=3 ("Target", "Other0", "Other1", "Other2"); each has a child class 4..=7;
+        // each child owns a Thing 8..=11.
+        let mut labels = Vec::new();
+        let mut names = Vec::new();
+        let mut forward: HashMap<(usize, String), Vec<usize>> = HashMap::new();
+
+        for i in 0..4 {
+            labels.push(vec!["Class".to_string()]);
+            names.push(if i == 0 {
+                "Target".to_string()
+            } else {
+                format!("Other{}", i - 1)
+            });
+        }
+        for i in 0..4 {
+            let child = 4 + i;
+            let thing = 8 + i;
+            labels.push(vec!["Class".to_string()]); // child class
+            names.push(format!("child{i}"));
+            forward.insert((i, "HAS_CHILD".to_string()), vec![child]);
+            forward.insert((child, "HAS_THING".to_string()), vec![thing]);
+        }
+        for i in 0..4 {
+            labels.push(vec!["Thing".to_string()]);
+            names.push(format!("thing{i}"));
+        }
+
+        SpyGraph {
+            labels,
+            names,
+            forward,
+            has_child_expansions: RefCell::new(Vec::new()),
+        }
+    }
+}
+
+impl GraphProvider for SpyGraph {
+    type NodeId = usize;
+
+    fn scan(&self, labels: &[String]) -> Vec<usize> {
+        (0..self.labels.len())
+            .filter(|id| labels.is_empty() || labels.iter().any(|l| self.matches_label(*id, l)))
+            .collect()
+    }
+
+    fn matches_label(&self, node: usize, label: &str) -> bool {
+        self.labels[node].iter().any(|l| l == label)
+    }
+
+    fn relationship_types(&self) -> Vec<String> {
+        vec!["HAS_CHILD".to_string(), "HAS_THING".to_string()]
+    }
+
+    fn expand(&self, node: usize, rel_type: &str) -> Vec<usize> {
+        if rel_type == "HAS_CHILD" {
+            self.has_child_expansions.borrow_mut().push(node);
+        }
+        self.forward
+            .get(&(node, rel_type.to_string()))
+            .cloned()
+            .unwrap_or_default()
+    }
+
+    fn rel_sources(&self, _rel_type: &str) -> Vec<usize> {
+        (0..self.labels.len()).collect()
+    }
+
+    fn property(&self, node: usize, prop: &str) -> CypherValue {
+        if prop == "name" {
+            CypherValue::Str(self.names[node].clone())
+        } else {
+            CypherValue::Null
+        }
+    }
+
+    fn node_id(&self, node: usize) -> String {
+        format!("n{node}")
+    }
+
+    fn label(&self, node: usize) -> String {
+        self.labels[node].first().cloned().unwrap_or_default()
+    }
+
+    fn name(&self, node: usize) -> String {
+        self.names[node].clone()
+    }
+}
+
+const QUERY: &str = "MATCH (t:Class)-[:HAS_CHILD]->(c:Class) \
+     MATCH (c)-[:HAS_THING]->(x:Thing) \
+     WHERE t.name = 'Target' \
+     RETURN x.name AS name";
+
+#[test]
+fn later_clause_predicate_prunes_earlier_scan() {
+    let graph = SpyGraph::new();
+    let result = execute(&graph, &parse(QUERY).unwrap()).unwrap();
+
+    // Correctness: only the Target subtree's Thing survives.
+    assert_eq!(result.columns, vec!["name".to_string()]);
+    assert_eq!(result.rows.len(), 1);
+    assert_eq!(result.rows[0][0], CypherValue::Str("thing0".to_string()));
+
+    // Optimization: `WHERE t.name = 'Target'` is pushed into the `t` scan, so the first clause
+    // expands `HAS_CHILD` from the single matching class only — not from every `Class` node.
+    let expanded = graph.has_child_expansions.borrow();
+    assert_eq!(
+        *expanded,
+        vec![0],
+        "expected HAS_CHILD to be expanded only from the matching 'Target' class (node 0), \
+         but it was expanded from {expanded:?} — the later-clause predicate was not pushed down"
+    );
+}

tests/pushdown_optional_safety.rs

@@ -0,0 +1,119 @@
+//! Regression test: cross-clause pushdown must NOT be applied inside an `OPTIONAL MATCH`.
+//!
+//! A single-variable predicate like `WHERE t IS NULL`, written on a non-optional clause, is eligible
+//! for pushdown. But if `t` is bound by an `OPTIONAL MATCH`, pushing `t IS NULL` into that optional
+//! expansion would prune every real `t` (no node satisfies `IS NULL`), null-filling every row and
+//! wrongly keeping them all. Pushdown must stay off during optional expansion; the anti-join must
+//! still work.
+
+use std::collections::HashMap;
+
+use cypher_parser::{CypherValue, GraphProvider, execute, parse};
+
+/// Two `Class` nodes — "Root" (no parent) and "Child" (`-[:PARENT]->` Root) — each owning one
+/// `Thing` via `HAS_THING`.
+struct Graph {
+    labels: Vec<Vec<String>>,
+    names: Vec<String>,
+    forward: HashMap<(usize, String), Vec<usize>>,
+}
+
+impl Graph {
+    fn new() -> Self {
+        // 0 = Root (Class), 1 = Child (Class), 2 = thing_root (Thing), 3 = thing_child (Thing).
+        let mut forward: HashMap<(usize, String), Vec<usize>> = HashMap::new();
+        forward.insert((1, "PARENT".to_string()), vec![0]); // Child -[:PARENT]-> Root
+        forward.insert((0, "HAS_THING".to_string()), vec![2]); // Root  -[:HAS_THING]-> thing_root
+        forward.insert((1, "HAS_THING".to_string()), vec![3]); // Child -[:HAS_THING]-> thing_child
+
+        Graph {
+            labels: vec![
+                vec!["Class".to_string()],
+                vec!["Class".to_string()],
+                vec!["Thing".to_string()],
+                vec!["Thing".to_string()],
+            ],
+            names: vec![
+                "Root".to_string(),
+                "Child".to_string(),
+                "thing_root".to_string(),
+                "thing_child".to_string(),
+            ],
+            forward,
+        }
+    }
+}
+
+impl GraphProvider for Graph {
+    type NodeId = usize;
+
+    fn scan(&self, labels: &[String]) -> Vec<usize> {
+        (0..self.labels.len())
+            .filter(|id| labels.is_empty() || labels.iter().any(|l| self.matches_label(*id, l)))
+            .collect()
+    }
+
+    fn matches_label(&self, node: usize, label: &str) -> bool {
+        self.labels[node].iter().any(|l| l == label)
+    }
+
+    fn relationship_types(&self) -> Vec<String> {
+        vec!["PARENT".to_string(), "HAS_THING".to_string()]
+    }
+
+    fn expand(&self, node: usize, rel_type: &str) -> Vec<usize> {
+        self.forward
+            .get(&(node, rel_type.to_string()))
+            .cloned()
+            .unwrap_or_default()
+    }
+
+    fn rel_sources(&self, _rel_type: &str) -> Vec<usize> {
+        (0..self.labels.len()).collect()
+    }
+
+    fn property(&self, node: usize, prop: &str) -> CypherValue {
+        if prop == "name" {
+            CypherValue::Str(self.names[node].clone())
+        } else {
+            CypherValue::Null
+        }
+    }
+
+    fn node_id(&self, node: usize) -> String {
+        format!("n{node}")
+    }
+
+    fn label(&self, node: usize) -> String {
+        self.labels[node].first().cloned().unwrap_or_default()
+    }
+
+    fn name(&self, node: usize) -> String {
+        self.names[node].clone()
+    }
+}
+
+/// `t IS NULL` (a pushable single-variable predicate on a non-optional clause) references `t`, which
+/// is bound by the OPTIONAL MATCH. If it were pushed into the optional expansion it would drop every
+/// real `t`, so both classes would survive; correctly, only Root (which has no PARENT) survives.
+#[test]
+fn is_null_predicate_not_pushed_into_optional_match() {
+    let graph = Graph::new();
+    let query = "MATCH (c:Class) \
+                 OPTIONAL MATCH (c)-[:PARENT]->(t:Class) \
+                 MATCH (c)-[:HAS_THING]->(x:Thing) \
+                 WHERE t IS NULL \
+                 RETURN x.name AS name";
+    let result = execute(&graph, &parse(query).unwrap()).unwrap();
+
+    let names: Vec<String> = result
+        .rows
+        .iter()
+        .map(|row| row[0].to_display_string())
+        .collect();
+    assert_eq!(
+        names,
+        vec!["thing_root".to_string()],
+        "only Root (no PARENT edge) should survive the t-IS-NULL anti-join"
+    );
+}