diff --git a/src/executor.rs b/src/executor.rs index 7dcff9f..4a8e235 100644 --- a/src/executor.rs +++ b/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(graph: &G, query: &Query) -> Result { /// expression's address, so `x IN ` 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>>>, - /// 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>>, + /// 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, } impl Executor<'_, G> { @@ -134,6 +142,12 @@ impl Executor<'_, G> { let mut rows: Vec> = vec![Row::new()]; let mut scope: Vec = 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 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 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, 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) -> HashMap> { +/// 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> { let mut map: HashMap> = 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()); + } } } } diff --git a/tests/cross_clause_pushdown.rs b/tests/cross_clause_pushdown.rs new file mode 100644 index 0000000..51dbdc7 --- /dev/null +++ b/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>, + names: Vec, + forward: HashMap<(usize, String), Vec>, + /// 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>, +} + +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> = 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 { + (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 { + vec!["HAS_CHILD".to_string(), "HAS_THING".to_string()] + } + + fn expand(&self, node: usize, rel_type: &str) -> Vec { + 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 { + (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" + ); +} diff --git a/tests/pushdown_optional_safety.rs b/tests/pushdown_optional_safety.rs new file mode 100644 index 0000000..92103ad --- /dev/null +++ b/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>, + names: Vec, + forward: HashMap<(usize, String), Vec>, +} + +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> = 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 { + (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 { + vec!["PARENT".to_string(), "HAS_THING".to_string()] + } + + fn expand(&self, node: usize, rel_type: &str) -> Vec { + self.forward + .get(&(node, rel_type.to_string())) + .cloned() + .unwrap_or_default() + } + + fn rel_sources(&self, _rel_type: &str) -> Vec { + (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 = 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" + ); +}