From bbf1524364e27dbbc3061a2044fd48e9ce27cc49 Mon Sep 17 00:00:00 2001 From: Mathew Beane Date: Tue, 20 Oct 2026 08:23:58 -0400 Subject: [PATCH] fix(rvlite): finish multi-row MATCH for RETURN columns, SET, DELETE and comma patterns #484 made RETURN iterate every MATCH row (#269), but the rest of the executor still reads bindings from the outer context, so: - MATCH with zero matches fell back to the outer context and RETURN n failed with VariableNotFound("n") instead of returning no rows. - RETURN n.name produced the column "?column?", and RETURN n.name, n.city collapsed both items into that one column. - MATCH ... SET and MATCH ... DETACH DELETE were no-ops, because SET and DELETE looked the variable up in the outer context, which MATCH no longer fills. - MATCH (p:Person), (c:Company) unioned the two pattern results instead of taking their cross product, so every row was missing a variable. SET, DELETE and RETURN now share one row_contexts() helper, the executor records whether a MATCH ran, property items get "var.prop" column names, and comma patterns combine as a cross product. Adds regression tests for each case alongside the #269 ones. Co-Authored-By: RuFlo --- crates/rvlite/src/cypher/executor.rs | 196 +++++++++++++++++---------- crates/rvlite/src/cypher/mod.rs | 156 +++++++++++++++++++++ 2 files changed, 281 insertions(+), 71 deletions(-) diff --git a/crates/rvlite/src/cypher/executor.rs b/crates/rvlite/src/cypher/executor.rs index 131bfe10b3..26816f6dc9 100644 --- a/crates/rvlite/src/cypher/executor.rs +++ b/crates/rvlite/src/cypher/executor.rs @@ -3,7 +3,7 @@ use super::ast::*; use super::graph_store::*; use serde::{Deserialize, Serialize}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use thiserror::Error; #[derive(Debug, Error)] @@ -101,17 +101,24 @@ impl ExecutionResult { /// Cypher query executor pub struct Executor<'a> { graph: &'a mut PropertyGraph, + /// Set once a MATCH has run in the current query, so an empty `matched_rows` + /// means "no rows" rather than "no MATCH". + matched: bool, } impl<'a> Executor<'a> { pub fn new(graph: &'a mut PropertyGraph) -> Self { - Self { graph } + Self { + graph, + matched: false, + } } /// Execute a parsed Cypher query pub fn execute(&mut self, query: &Query) -> Result { let mut context = ExecutionContext::new(); let mut result = None; + self.matched = false; for statement in &query.statements { result = Some(self.execute_statement(statement, &mut context)?); @@ -256,11 +263,21 @@ impl<'a> Executor<'a> { clause: &MatchClause, context: &mut ExecutionContext, ) -> Result { - let mut matches = Vec::new(); + // Comma-separated patterns combine as a cross product; a variable that + // appears in more than one pattern must bind to the same element. + let mut matches = vec![ExecutionContext::new()]; for pattern in &clause.patterns { let pattern_matches = self.match_pattern(pattern)?; - matches.extend(pattern_matches); + let mut combined = Vec::new(); + for left in &matches { + for right in &pattern_matches { + if let Some(ctx) = merge_bindings(left, right) { + combined.push(ctx); + } + } + } + matches = combined; } // Apply WHERE filter if present @@ -273,10 +290,31 @@ impl<'a> Executor<'a> { // Store each matched binding set as a separate row so RETURN can iterate all of them context.matched_rows = matches.into_iter().map(|ctx| ctx.variables).collect(); + self.matched = true; Ok(ExecutionResult::new(vec![])) } + /// One context per matched row, each holding the outer bindings plus that + /// row's bindings. Without a preceding MATCH this is the outer context alone. + fn row_contexts(&self, context: &ExecutionContext) -> Vec { + if !self.matched { + return vec![context.clone()]; + } + context + .matched_rows + .iter() + .map(|bindings| { + let mut row_ctx = ExecutionContext::new(); + row_ctx.variables = context.variables.clone(); + for (k, v) in bindings { + row_ctx.bind(k.clone(), v.clone()); + } + row_ctx + }) + .collect() + } + fn match_pattern(&self, pattern: &Pattern) -> Result, ExecutionError> { match pattern { Pattern::Node(node_pattern) => self.match_node_pattern(node_pattern), @@ -421,13 +459,7 @@ impl<'a> Executor<'a> { ) -> Result { let mut columns = Vec::new(); for item in &clause.items { - let col_name = item - .alias - .clone() - .unwrap_or_else(|| match &item.expression { - Expression::Variable(var) => var.clone(), - _ => "?column?".to_string(), - }); + let col_name = column_name(item); if !columns.contains(&col_name) { columns.push(col_name); } @@ -435,40 +467,14 @@ impl<'a> Executor<'a> { let mut result = ExecutionResult::new(columns.clone()); - // If MATCH produced multiple rows, iterate each row; otherwise use the current context - let row_bindings: Vec<&HashMap> = if !context.matched_rows.is_empty() - { - context.matched_rows.iter().collect() - } else { - vec![&context.variables] - }; - - for bindings in row_bindings { - // Build a temporary context for this row by merging matched bindings with outer context - let mut row_ctx = ExecutionContext::new(); - for (k, v) in &context.variables { - row_ctx.bind(k.clone(), v.clone()); - } - for (k, v) in bindings { - row_ctx.bind(k.clone(), v.clone()); - } - + for row_ctx in self.row_contexts(context) { let mut row = HashMap::new(); for col_name in &columns { // Find the expression for this column let expr = clause .items .iter() - .find(|item| { - let name = item - .alias - .clone() - .unwrap_or_else(|| match &item.expression { - Expression::Variable(var) => var.clone(), - _ => "?column?".to_string(), - }); - &name == col_name - }) + .find(|item| &column_name(item) == col_name) .map(|item| &item.expression); if let Some(expr) = expr { @@ -487,24 +493,26 @@ impl<'a> Executor<'a> { clause: &SetClause, context: &ExecutionContext, ) -> Result { - for item in &clause.items { - match item { - SetItem::Property { - variable, - property, - value, - } => { - let val = self.evaluate_expression(value, context)?; - if let Some(ContextValue::Node(node)) = context.get(variable) { - if let Some(node_mut) = self.graph.get_node_mut(&node.id) { - node_mut.set_property(property.clone(), val); + for row_ctx in self.row_contexts(context) { + for item in &clause.items { + match item { + SetItem::Property { + variable, + property, + value, + } => { + let val = self.evaluate_expression(value, &row_ctx)?; + if let Some(ContextValue::Node(node)) = row_ctx.get(variable) { + if let Some(node_mut) = self.graph.get_node_mut(&node.id) { + node_mut.set_property(property.clone(), val); + } } } - } - _ => { - return Err(ExecutionError::UnsupportedOperation( - "Only property SET supported".to_string(), - )) + _ => { + return Err(ExecutionError::UnsupportedOperation( + "Only property SET supported".to_string(), + )) + } } } } @@ -517,24 +525,34 @@ impl<'a> Executor<'a> { clause: &DeleteClause, context: &ExecutionContext, ) -> Result { - for expr in &clause.expressions { - if let Expression::Variable(var) = expr { - if let Some(ctx_val) = context.get(var) { - match ctx_val { - ContextValue::Node(node) => { - if clause.detach { - self.graph.delete_node(&node.id)?; - } else { - return Err(ExecutionError::ExecutionError( - "Cannot delete node with relationships without DETACH" - .to_string(), - )); + // Cross products can bind the same element in several rows; delete it once. + let mut deleted_nodes = HashSet::new(); + let mut deleted_edges = HashSet::new(); + + for row_ctx in self.row_contexts(context) { + for expr in &clause.expressions { + if let Expression::Variable(var) = expr { + if let Some(ctx_val) = row_ctx.get(var) { + match ctx_val { + ContextValue::Node(node) => { + if clause.detach { + if deleted_nodes.insert(node.id.clone()) { + self.graph.delete_node(&node.id)?; + } + } else { + return Err(ExecutionError::ExecutionError( + "Cannot delete node with relationships without DETACH" + .to_string(), + )); + } } + ContextValue::Edge(edge) => { + if deleted_edges.insert(edge.id.clone()) { + self.graph.delete_edge(&edge.id)?; + } + } + _ => {} } - ContextValue::Edge(edge) => { - self.graph.delete_edge(&edge.id)?; - } - _ => {} } } } @@ -653,3 +671,39 @@ impl<'a> Executor<'a> { } } } + +/// Column name for a RETURN item: its alias, else `n` or `n.prop`. +fn column_name(item: &ReturnItem) -> String { + if let Some(alias) = &item.alias { + return alias.clone(); + } + match &item.expression { + Expression::Variable(var) => var.clone(), + Expression::Property { object, property } => match &**object { + Expression::Variable(var) => format!("{}.{}", var, property), + _ => "?column?".to_string(), + }, + _ => "?column?".to_string(), + } +} + +/// Combine two binding sets, or `None` if they bind one variable to different elements. +fn merge_bindings(left: &ExecutionContext, right: &ExecutionContext) -> Option { + let mut merged = left.clone(); + for (var, value) in &right.variables { + if let Some(existing) = merged.get(var) { + let same = match (existing, value) { + (ContextValue::Node(a), ContextValue::Node(b)) => a.id == b.id, + (ContextValue::Edge(a), ContextValue::Edge(b)) => a.id == b.id, + (ContextValue::Value(a), ContextValue::Value(b)) => a == b, + _ => false, + }; + if !same { + return None; + } + } else { + merged.bind(var.clone(), value.clone()); + } + } + Some(merged) +} diff --git a/crates/rvlite/src/cypher/mod.rs b/crates/rvlite/src/cypher/mod.rs index 3208dc50c5..775bf7cd46 100644 --- a/crates/rvlite/src/cypher/mod.rs +++ b/crates/rvlite/src/cypher/mod.rs @@ -230,6 +230,75 @@ mod tests { assert_eq!(stats.edge_count, 1); } + /// Issue #269: MATCH returns one row per match, not only the last one. + #[test] + fn test_match_returns_multiple_rows() { + let mut engine = CypherEngine::new(); + run( + &mut engine, + "CREATE (a:Person {name: 'Alice'}), (b:Person {name: 'Bob'}), (c:Person {name: 'Charlie'})", + ); + + let result = run(&mut engine, "MATCH (n:Person) RETURN n"); + + assert_eq!(result.rows.len(), 3); + assert_eq!(result.columns, vec!["n"]); + } + + /// RETURN n.name names its column "n.name" and yields one value per row. + #[test] + fn test_match_return_properties() { + let mut engine = CypherEngine::new(); + run( + &mut engine, + "CREATE (a:Person {name: 'Alice'}), (b:Person {name: 'Bob'})", + ); + + let result = run(&mut engine, "MATCH (n:Person) RETURN n.name"); + + assert_eq!(result.columns, vec!["n.name"]); + assert_eq!(string_column(&result, "n.name"), vec!["Alice", "Bob"]); + } + + #[test] + fn test_match_where_filter() { + let mut engine = CypherEngine::new(); + run( + &mut engine, + "CREATE (a:Person {name: 'Alice', age: 30}), (b:Person {name: 'Bob', age: 25}), (c:Person {name: 'Charlie', age: 35})", + ); + + let result = run( + &mut engine, + "MATCH (n:Person) WHERE n.age > 28 RETURN n.name", + ); + + assert_eq!(string_column(&result, "n.name"), vec!["Alice", "Charlie"]); + } + + /// A MATCH with no matches gives zero rows, not an unbound-variable error. + #[test] + fn test_match_no_results() { + let mut engine = CypherEngine::new(); + run(&mut engine, "CREATE (a:Person {name: 'Alice'})"); + + let result = run(&mut engine, "MATCH (n:Animal) RETURN n"); + + assert_eq!(result.rows.len(), 0); + } + + #[test] + fn test_match_many_nodes() { + let mut engine = CypherEngine::new(); + for i in 0..100 { + run(&mut engine, &format!("CREATE (n:Item {{id: {}}})", i)); + } + + let result = run(&mut engine, "MATCH (n:Item) RETURN n"); + + assert_eq!(result.rows.len(), 100); + } + #[test] fn test_match_nodes() { let mut engine = CypherEngine::new(); @@ -263,4 +332,91 @@ mod tests { assert!(result.is_ok(), "Failed to parse: {}", query); } } + + fn run(engine: &mut CypherEngine, query: &str) -> ExecutionResult { + let ast = parse_cypher(query).unwrap(); + let mut executor = Executor::new(&mut engine.graph); + executor.execute(&ast).unwrap() + } + + fn string_column(result: &ExecutionResult, column: &str) -> Vec { + let mut values: Vec = result + .rows + .iter() + .filter_map(|row| match row.get(column) { + Some(ContextValue::Value(Value::String(s))) => Some(s.clone()), + _ => None, + }) + .collect(); + values.sort(); + values + } + + #[test] + fn test_return_multiple_property_columns() { + let mut engine = CypherEngine::new(); + run( + &mut engine, + "CREATE (a:Person {name: 'Alice', city: 'Oslo'}), (b:Person {name: 'Bob', city: 'Rome'})", + ); + + let result = run(&mut engine, "MATCH (n:Person) RETURN n.name, n.city"); + + assert_eq!(result.columns, vec!["n.name", "n.city"]); + assert_eq!(string_column(&result, "n.name"), vec!["Alice", "Bob"]); + assert_eq!(string_column(&result, "n.city"), vec!["Oslo", "Rome"]); + } + + #[test] + fn test_match_set_updates_every_row() { + let mut engine = CypherEngine::new(); + run( + &mut engine, + "CREATE (a:Person {name: 'Alice'}), (b:Person {name: 'Bob'}), (c:Person {name: 'Charlie'})", + ); + + run(&mut engine, "MATCH (n:Person) SET n.seen = 'yes'"); + + let result = run( + &mut engine, + "MATCH (n:Person) WHERE n.seen = 'yes' RETURN n.name", + ); + assert_eq!( + string_column(&result, "n.name"), + vec!["Alice", "Bob", "Charlie"] + ); + } + + #[test] + fn test_match_detach_delete_every_row() { + let mut engine = CypherEngine::new(); + run( + &mut engine, + "CREATE (a:Person {name: 'Alice'}), (b:Person {name: 'Bob'}), (c:Item {id: 1})", + ); + + run(&mut engine, "MATCH (n:Person) DETACH DELETE n"); + + assert_eq!(engine.graph.stats().node_count, 1); + assert!(engine.graph.find_nodes_by_label("Person").is_empty()); + } + + #[test] + fn test_match_comma_patterns_cross_product() { + let mut engine = CypherEngine::new(); + run( + &mut engine, + "CREATE (a:Person {name: 'Alice'}), (b:Person {name: 'Bob'}), (x:Company {name: 'Acme'}), (y:Company {name: 'Initech'}), (z:Company {name: 'Umbrella'})", + ); + + let result = run( + &mut engine, + "MATCH (p:Person), (c:Company) RETURN p.name, c.name", + ); + + assert_eq!(result.rows.len(), 6); + for row in &result.rows { + assert!(row.contains_key("p.name") && row.contains_key("c.name")); + } + } }