feat(cypher): UNWIND MATCH SET/DELETE + DELETE RETURN (SPA-415)#411
Open
evuk79 wants to merge 4 commits into
Open
feat(cypher): UNWIND MATCH SET/DELETE + DELETE RETURN (SPA-415)#411evuk79 wants to merge 4 commits into
evuk79 wants to merge 4 commits into
Conversation
Adds two new Cypher capabilities to SparrowDB:
1. UNWIND ... MATCH ... SET/DELETE [RETURN ...]
- New AST variant: UnwindMatchMutateStatement
- Parser: parse_unwind() extended to handle MATCH + mutation tail
- Binder: binds MATCH patterns, UNWIND expr uses
- Engine: is_mutation() returns true for UnwindMatchMutate
- db.rs: execute_unwind_match_mutate_with_params() evaluates
the UNWIND list and applies mutations per-row within a single
write transaction (eliminates N+1 query parse overhead)
2. DELETE ... RETURN count(n)
- MatchMutateStatement gains optional return_clause field
- Parser: parse_match_or_match_mutate() parses RETURN after SET/DELETE
- db.rs: build_mutate_return() helper returns mutation count
- All three mutation paths (direct, deadline, with-params) updated
These capabilities enable awknow-rs to replace N indidvidual
MATCH...SET queries with a single UNWIND query, and replace
COUNT-then-DELETE with a single DELETE...RETURN query.
Closes the N+1 and COUNT+DELETE anti-patterns in SparrowGraphAccess.
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds AST, parser, binder, engine routing, and DB execution to support UNWIND-driven batched MATCH+SET/DELETE with optional trailing RETURN; threads RETURN into MatchMutate, refactors SET parsing for UNWIND, marks UnwindMatchMutate as mutation, and implements UNWIND execution plus count-return helpers. ChangesUNWIND...MATCH...SET/DELETE Mutations with Optional RETURN
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for UNWIND ... MATCH ... SET/DELETE statements (SPA-415) to enable batched mutations via list parameters, as well as optional RETURN clauses after mutations. The feedback identifies critical issues with parameter resolution, as the params map is currently discarded during execution, preventing parameters from being resolved in where_clause filters or SET mutations. To address this, the params map should be passed down through execute_unwind_mutate_inner to the execution engine and resolve_set_value. Additionally, the parser can be simplified by reusing the new parse_optional_return helper, and the fallback column header for mutation returns should be standardized to a generic name like "count(n)" instead of dynamically embedding the count value.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+1650
to
+1657
| fn execute_unwind_match_mutate_with_params( | ||
| &self, | ||
| umm: &sparrowdb_cypher::ast::UnwindMatchMutateStatement, | ||
| params: &HashMap<String, sparrowdb_execution::Value>, | ||
| ) -> Result<QueryResult> { | ||
| let list = eval_unwind_list(&umm.expr, params)?; | ||
| self.execute_unwind_mutate_inner(umm, list.as_slice()) | ||
| } |
There was a problem hiding this comment.
The params map is evaluated to resolve the UNWIND list, but is then discarded. Any parameter references in the where_clause or mutations of the UnwindMatchMutateStatement will fail to resolve. Pass params down to execute_unwind_mutate_inner to fix this.
fn execute_unwind_match_mutate_with_params(
&self,
umm: &sparrowdb_cypher::ast::UnwindMatchMutateStatement,
params: &HashMap<String, sparrowdb_execution::Value>,
) -> Result<QueryResult> {
let list = eval_unwind_list(&umm.expr, params)?;
self.execute_unwind_mutate_inner(umm, list.as_slice(), Some(params))
}
Comment on lines
+1660
to
+1679
| fn execute_unwind_match_mutate( | ||
| &self, | ||
| umm: &sparrowdb_cypher::ast::UnwindMatchMutateStatement, | ||
| ) -> Result<QueryResult> { | ||
| let list = match &umm.expr { | ||
| sparrowdb_cypher::ast::Expr::List(items) => items | ||
| .iter() | ||
| .map(|e| { | ||
| let sv = expr_to_value(e); | ||
| Ok(storage_value_to_exec(&sv)) | ||
| }) | ||
| .collect::<Result<Vec<_>>>()?, | ||
| _ => { | ||
| return Err(Error::InvalidArgument( | ||
| "UNWIND MATCH SET/DELETE without parameters requires a list literal".into(), | ||
| )); | ||
| } | ||
| }; | ||
| self.execute_unwind_mutate_inner(umm, list.as_slice()) | ||
| } |
There was a problem hiding this comment.
Update the call to execute_unwind_mutate_inner to pass None for the optional params map when executing without parameters.
fn execute_unwind_match_mutate(
&self,
umm: &sparrowdb_cypher::ast::UnwindMatchMutateStatement,
) -> Result<QueryResult> {
let list = match &umm.expr {
sparrowdb_cypher::ast::Expr::List(items) => items
.iter()
.map(|e| {
let sv = expr_to_value(e);
Ok(storage_value_to_exec(&sv))
})
.collect::<Result<Vec<_>>>()?,
_ => {
return Err(Error::InvalidArgument(
"UNWIND MATCH SET/DELETE without parameters requires a list literal".into(),
));
}
};
self.execute_unwind_mutate_inner(umm, list.as_slice(), None)
}
Comment on lines
+1682
to
+1698
| fn execute_unwind_mutate_inner( | ||
| &self, | ||
| umm: &sparrowdb_cypher::ast::UnwindMatchMutateStatement, | ||
| list: &[sparrowdb_execution::Value], | ||
| ) -> Result<QueryResult> { | ||
| if list.is_empty() { | ||
| return Ok(QueryResult::empty(vec![])); | ||
| } | ||
|
|
||
| let mut tx = self.begin_write()?; | ||
| let csrs = self.cached_csr_map(); | ||
| let engine = Engine::new( | ||
| NodeStore::open(&self.inner.path)?, | ||
| self.catalog_snapshot(), | ||
| csrs, | ||
| &self.inner.path, | ||
| ); |
There was a problem hiding this comment.
Update execute_unwind_mutate_inner to accept the optional params map and configure the engine with it. This ensures parameter references in the where_clause can be resolved correctly.
fn execute_unwind_mutate_inner(
&self,
umm: &sparrowdb_cypher::ast::UnwindMatchMutateStatement,
list: &[sparrowdb_execution::Value],
params: Option<&HashMap<String, sparrowdb_execution::Value>>,
) -> Result<QueryResult> {
if list.is_empty() {
return Ok(QueryResult::empty(vec![]));
}
let mut tx = self.begin_write()?;
let csrs = self.cached_csr_map();
let mut engine = Engine::new(
NodeStore::open(&self.inner.path)?,
self.catalog_snapshot(),
csrs,
&self.inner.path,
);
if let Some(p) = params {
engine = engine.with_params(p.clone());
}
Comment on lines
+1734
to
+1741
| for mutation in &umm.mutations { | ||
| match mutation { | ||
| sparrowdb_cypher::ast::Mutation::Set { prop, value, .. } => { | ||
| let sv = resolve_set_value(value, &umm.alias, row); | ||
| for node_id in &matching_ids { | ||
| tx.set_property(*node_id, prop, sv.clone())?; | ||
| } | ||
| } |
There was a problem hiding this comment.
Pass params to resolve_set_value and handle the returned Result to support parameter references in SET mutations.
for mutation in &umm.mutations {
match mutation {
sparrowdb_cypher::ast::Mutation::Set { prop, value, .. } => {
let sv = resolve_set_value(value, &umm.alias, row, params)?;
for node_id in &matching_ids {
tx.set_property(*node_id, prop, sv.clone())?;
}
}
Comment on lines
+3189
to
+3206
| fn resolve_set_value( | ||
| value: &sparrowdb_cypher::ast::Expr, | ||
| alias: &str, | ||
| row: &[(String, sparrowdb_execution::Value)], | ||
| ) -> Value { | ||
| use sparrowdb_cypher::ast::Expr; | ||
| if let Expr::PropAccess { var, prop } = value { | ||
| if var == alias { | ||
| for (key, val) in row { | ||
| if key == prop { | ||
| return exec_value_to_storage(val); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // Fall back to standard literal evaluation. | ||
| expr_to_value(value) | ||
| } |
There was a problem hiding this comment.
Update resolve_set_value to accept params and fall back to expr_to_value_with_params when resolving non-alias expressions. This ensures global parameters can be resolved inside the SET clause.
fn resolve_set_value(
value: &sparrowdb_cypher::ast::Expr,
alias: &str,
row: &[(String, sparrowdb_execution::Value)],
params: Option<&HashMap<String, sparrowdb_execution::Value>>,
) -> Result<Value> {
use sparrowdb_cypher::ast::Expr;
if let Expr::PropAccess { var, prop } = value {
if var == alias {
for (key, val) in row {
if key == prop {
return Ok(exec_value_to_storage(val));
}
}
}
}
// Fall back to standard literal evaluation.
if let Some(p) = params {
expr_to_value_with_params(value, p)
} else {
Ok(expr_to_value(value))
}
}
Comment on lines
+1759
to
+1766
| let return_clause = if matches!(self.peek(), Token::Return) { | ||
| self.advance(); | ||
| let items = self.parse_return_items()?; | ||
| Some(ReturnClause { items }) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
There was a problem hiding this comment.
The parser can reuse the newly introduced parse_optional_return helper instead of duplicating the logic for parsing optional RETURN clauses.
// Optional RETURN clause after mutations.
let return_clause = self.parse_optional_return()?;
Comment on lines
+1629
to
+1633
| if rc.items.len() == 1 { | ||
| let col = rc.items[0] | ||
| .alias | ||
| .clone() | ||
| .unwrap_or_else(|| format!("count({})", count)); |
There was a problem hiding this comment.
The fallback column name format!("count({})", count) uses the actual count value (e.g., count(5)) as the column header instead of a generic header like count(n). This is non-standard for Cypher query results.
if rc.items.len() == 1 {
let col = rc.items[0]
.alias
.clone()
.unwrap_or_else(|| "count(n)".to_string());![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/sparrowdb/src/db.rs (1)
1499-1514:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSome mutation exits still bypass
build_mutate_return().These branches return
QueryResult::empty(...)before the return helper runs.... RETURN count(...)therefore yields no row instead of0for empty matches / empty UNWIND lists, and the params/deadline edge-delete paths still drop the count result after a successful delete.Also applies to: 1687-1689, 1864-1879
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sparrowdb/src/db.rs` around lines 1499 - 1514, Some mutation branches (e.g., the edge-delete path using is_edge_delete_mutation, the empty matching_ids early-return after scan_match_mutate, and other similar mutation exits) return QueryResult::empty(...) before calling build_mutate_return(), causing RETURN count(...) to be lost; change those early returns to call build_mutate_return(...) and return its result instead (preserving existing actions like tx.commit(), self.invalidate_csr_map(), self.invalidate_catalog() where applicable). Specifically, in the edge-delete branch (where scan_match_mutate_edges, tx.delete_edge, tx.commit, invalidate_csr_map/catalog currently occur) call build_mutate_return(...) after committing and invalidation and return that QueryResult; likewise replace the empty-matches return after scan_match_mutate() with a call to build_mutate_return(...) (passing the empty id list or appropriate context) so UNWIND/COUNT and other mutation-return semantics are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/sparrowdb-cypher/src/parser.rs`:
- Around line 1639-1641: The code currently routes any UNWIND followed by
Token::Match into parse_unwind_match_mutate, which breaks read queries like
UNWIND ... MATCH ... RETURN; change the branch to perform a lookahead after
Token::Match and only call parse_unwind_match_mutate when the MATCH clause is
actually followed by mutation keywords (e.g., Token::Set, Token::Delete,
Token::Create, Token::Merge or other mutation-specific tokens); otherwise
continue the normal read-query parse path (i.e., do not call
parse_unwind_match_mutate). Implement the lookahead using the parser's existing
peek/peek_n functionality (or by cloning the parser cursor) and reference
Token::Match and the parse_unwind_match_mutate call when making the conditional.
In `@crates/sparrowdb/src/db.rs`:
- Around line 1691-1729: The UNWIND loop is using Engine::new (created against
committed storage) and only inlining pattern properties via
resolve_pattern_props, so scan_match_mutate is unaware of tx.pending_ops and the
element-specific WHERE bindings (umm.where_clause) — causing re-matches of rows
already mutated in the current transaction. Fix by making scans tx-aware: either
create or derive an Engine/scan context that includes the active transaction's
pending_ops (the tx returned by begin_write()) before calling
engine.scan_match_mutate, and also resolve/instantiate umm.where_clause per
element (similar to how match_patterns are resolved) so element-specific WHERE
predicates are evaluated against the resolved row; update the code around
begin_write, Engine::new, resolve_pattern_props, the resolved_mm construction,
and the call to scan_match_mutate to pass the tx-aware engine/context.
- Around line 1625-1639: The code in build_mutate_return currently treats any
single-item RETURN as a count and even uses the runtime count to synthesize the
column label; change this to inspect the RETURN AST: only manufacture the count
row when rc.items.len() == 1 and rc.items[0] is a count aggregation expression
(e.g., Count/CountStar node) — otherwise fall back to returning an empty
QueryResult or appropriate behavior; when creating the column label use the
RETURN item's alias or derive a label from the expression text (not the runtime
count) via rc.items[0].alias.unwrap_or_else(|| format!("count({})",
<expression_identifier_or_render_of_count>)) so supported RETURN count(n) keeps
its correct column name.
- Around line 743-745: The UNWIND mutation branch in execute_with_timeout
currently calls execute_unwind_match_mutate(umm) without forwarding the caller's
deadline, so long-running UNWIND ... MATCH ... SET/DELETE operations can ignore
timeouts; update the dispatch in execute_with_timeout to pass the deadline
through to execute_unwind_match_mutate (and update the
execute_unwind_match_mutate signature/implementations as needed) so the deadline
parameter is honored for Statement::UnwindMatchMutate paths.
---
Outside diff comments:
In `@crates/sparrowdb/src/db.rs`:
- Around line 1499-1514: Some mutation branches (e.g., the edge-delete path
using is_edge_delete_mutation, the empty matching_ids early-return after
scan_match_mutate, and other similar mutation exits) return
QueryResult::empty(...) before calling build_mutate_return(), causing RETURN
count(...) to be lost; change those early returns to call
build_mutate_return(...) and return its result instead (preserving existing
actions like tx.commit(), self.invalidate_csr_map(), self.invalidate_catalog()
where applicable). Specifically, in the edge-delete branch (where
scan_match_mutate_edges, tx.delete_edge, tx.commit, invalidate_csr_map/catalog
currently occur) call build_mutate_return(...) after committing and invalidation
and return that QueryResult; likewise replace the empty-matches return after
scan_match_mutate() with a call to build_mutate_return(...) (passing the empty
id list or appropriate context) so UNWIND/COUNT and other mutation-return
semantics are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 967bf63d-df23-436a-b973-6be26341ecb1
📒 Files selected for processing (5)
crates/sparrowdb-cypher/src/ast.rscrates/sparrowdb-cypher/src/binder.rscrates/sparrowdb-cypher/src/parser.rscrates/sparrowdb-execution/src/engine/mod.rscrates/sparrowdb/src/db.rs
Comment on lines
+1691
to
+1729
| let mut tx = self.begin_write()?; | ||
| let csrs = self.cached_csr_map(); | ||
| let engine = Engine::new( | ||
| NodeStore::open(&self.inner.path)?, | ||
| self.catalog_snapshot(), | ||
| csrs, | ||
| &self.inner.path, | ||
| ); | ||
| let mut total_mutated: u64 = 0; | ||
| let mut has_detach_delete = false; | ||
|
|
||
| for element in list { | ||
| // Each element must be a Map (e.g. {id: "m1", score: 0.5}). | ||
| let row = match element { | ||
| sparrowdb_execution::Value::Map(entries) => entries, | ||
| _ => { | ||
| return Err(Error::InvalidArgument(format!( | ||
| "UNWIND element must be a map (e.g. {{id: '...', score: 0.5}}), got {:?}", | ||
| std::mem::discriminant(element) | ||
| ))); | ||
| } | ||
| }; | ||
|
|
||
| // Build a temporary MatchMutateStatement with inline property | ||
| // filters resolved from the row values. | ||
| let resolved_patterns: Vec<sparrowdb_cypher::ast::PathPattern> = umm | ||
| .match_patterns | ||
| .iter() | ||
| .map(|pat| resolve_pattern_props(pat, &umm.alias, row)) | ||
| .collect(); | ||
|
|
||
| let resolved_mm = sparrowdb_cypher::ast::MatchMutateStatement { | ||
| match_patterns: resolved_patterns, | ||
| where_clause: umm.where_clause.clone(), | ||
| mutations: umm.mutations.clone(), | ||
| return_clause: None, | ||
| }; | ||
|
|
||
| let matching_ids = engine.scan_match_mutate(&resolved_mm)?; |
There was a problem hiding this comment.
UNWIND rows are not scanned against a row-specific, tx-aware statement.
Only inline pattern props are resolved per element. where_clause is cloned unchanged, and the shared Engine keeps scanning committed storage while earlier iterations only exist in tx.pending_ops. That means WHERE <alias>.<prop> ... predicates never bind, and later UNWIND rows can re-match rows that earlier iterations already updated or deleted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sparrowdb/src/db.rs` around lines 1691 - 1729, The UNWIND loop is
using Engine::new (created against committed storage) and only inlining pattern
properties via resolve_pattern_props, so scan_match_mutate is unaware of
tx.pending_ops and the element-specific WHERE bindings (umm.where_clause) —
causing re-matches of rows already mutated in the current transaction. Fix by
making scans tx-aware: either create or derive an Engine/scan context that
includes the active transaction's pending_ops (the tx returned by begin_write())
before calling engine.scan_match_mutate, and also resolve/instantiate
umm.where_clause per element (similar to how match_patterns are resolved) so
element-specific WHERE predicates are evaluated against the resolved row; update
the code around begin_write, Engine::new, resolve_pattern_props, the resolved_mm
construction, and the call to scan_match_mutate to pass the tx-aware
engine/context.
added 3 commits
June 10, 2026 08:57
- Pass params through to execute_unwind_mutate_inner so $param references
in where_clause and SET mutations resolve correctly (Gemini review).
- resolve_set_value now accepts optional params and uses
expr_to_value_with_params for $param-aware fallback.
- Engine configured with params in inner execution path.
- Parser reuses parse_optional_return() in parse_unwind_match_mutate
instead of duplicating RETURN parsing logic.
- build_mutate_return uses 'count(n)' as fallback column name instead
of 'count({})' (non-standard Cypher header).
- Replace std::mem::discriminant with Display-based error messages
(Value does not implement Discriminant).
… count check - Parser: UNWIND MATCH now dispatches correctly — only routes to mutation parsing when followed by SET/DELETE/DETACH/WHERE. Read queries like UNWIND ... MATCH ... RETURN continue to use the read pipeline path. (CodeRabbit: parser regression fix) - Deadline: execute_with_timeout now passes deadline to execute_unwind_match_mutate_deadline, which checks elapsed time between UNWIND row iterations (fixes ryaker#310). - build_mutate_return: now only honours count(n)/count(*) RETURN expressions; arbitrary single-item RETURN clauses safely produce empty results instead of fabricating a count row. - Documented UNWIND row visibility limitation: each iteration scans against committed storage (standard Cypher UNWIND semantics).
The parser's parse_set_items() guard rejected non-literal SET values at parse time. This broke UNWIND ... MATCH ... SET queries where the SET value references the unwind alias (e.g. SET n.score = row.score). Added parse_set_items_allow_prop() that permits PropAccess expressions in SET values. Used by parse_unwind_match_mutate_tail(); the standard MATCH...SET path retains the strict literal-only guard. Fixes test failures in awknow-rs sparrow_graph::test_apply_decay_*.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds two new Cypher capabilities to SparrowDB:
UNWIND ... MATCH ... SET/DELETE [RETURN ...]
the UNWIND list and applies mutations per-row within a single
write transaction (eliminates N+1 query parse overhead)
DELETE ... RETURN count(n)
These capabilities enable awknow-rs to replace N indidvidual
MATCH...SET queries with a single UNWIND query, and replace
COUNT-then-DELETE with a single DELETE...RETURN query.
Closes the N+1 and COUNT+DELETE anti-patterns in SparrowGraphAccess.
Summary by CodeRabbit