A SET applies its writes in the order they were written (#1896)

src/query/ast.rs

@@ -1045,16 +1045,52 @@ pub struct DeleteClause {
 /// SET clause
 #[derive(Debug, Clone, PartialEq)]
 pub struct SetClause {
-    /// Items to set
-    pub items: Vec<SetItem>,
-    /// Labels to add: `SET n:Admin`, `SET n:A:B`.
+    /// The items, in the order they were written.
     ///
-    /// Kept beside `items` rather than folded into it because `SetItem` is a
-    /// struct read by field in several places, and an enum would touch all of
-    /// them for no gain. `RemoveItem` is an enum because it always was.
-    pub label_items: Vec<SetLabelItem>,
-    /// Whole-entity assignment: `SET n = {…}` and `SET n += {…}`.
-    pub entity_items: Vec<SetEntityItem>,
+    /// One sequence rather than a vector per kind, because the order between
+    /// kinds is part of the meaning: `SET n = {a: 1}, n.k = 7` keeps `k` and
+    /// `SET n.k = 7, n = {a: 1}` does not. Three vectors kept each kind's own
+    /// order and lost the order between them, so the executor had to pick one
+    /// -- and whichever it picked, the other way round was silently wrong
+    /// (#1896).
+    pub ops: Vec<SetOp>,
+}
+
+impl SetClause {
+    /// `n.prop = value` items, in order.
+    pub fn items(&self) -> impl Iterator<Item = &SetItem> {
+        self.ops.iter().filter_map(|op| match op {
+            SetOp::Property(i) => Some(i),
+            _ => None,
+        })
+    }
+
+    /// `n:Label` items, in order.
+    pub fn label_items(&self) -> impl Iterator<Item = &SetLabelItem> {
+        self.ops.iter().filter_map(|op| match op {
+            SetOp::Labels(i) => Some(i),
+            _ => None,
+        })
+    }
+
+    /// `n = …` and `n += …` items, in order.
+    pub fn entity_items(&self) -> impl Iterator<Item = &SetEntityItem> {
+        self.ops.iter().filter_map(|op| match op {
+            SetOp::Entity(i) => Some(i),
+            _ => None,
+        })
+    }
+}
+
+/// One item of a `SET` clause.
+#[derive(Debug, Clone, PartialEq)]
+pub enum SetOp {
+    /// `n.prop = value`
+    Property(SetItem),
+    /// `n:Admin`, `n:A:B`
+    Labels(SetLabelItem),
+    /// `n = {…}` (replace) or `n += {…}` (merge)
+    Entity(SetEntityItem),
 }
 
 /// `SET n = <expr>` (replace every property) or `SET n += <expr>` (merge).

src/query/executor/mod.rs

@@ -1089,11 +1089,12 @@ fn substitute_set(
     sc: &mut crate::query::ast::SetClause,
     p: &HashMap<String, crate::graph::PropertyValue>,
 ) -> ExecutionResult<()> {
-    for item in &mut sc.items {
-        substitute_expr(&mut item.value, p)?;
-    }
-    for item in &mut sc.entity_items {
-        substitute_expr(&mut item.value, p)?;
+    for op in &mut sc.ops {
+        match op {
+            crate::query::ast::SetOp::Property(item) => substitute_expr(&mut item.value, p)?,
+            crate::query::ast::SetOp::Entity(item) => substitute_expr(&mut item.value, p)?,
+            crate::query::ast::SetOp::Labels(_) => {}
+        }
     }
     Ok(())
 }

src/query/executor/operator.rs

@@ -20611,25 +20611,50 @@ fn apply_entity_assignment(
     Ok(())
 }
 
+/// One write of a `SET` clause, as `SetPropertyOperator` applies it.
+#[derive(Debug, Clone)]
+pub enum SetWrite {
+    /// `variable.property = value`
+    Property(String, String, Expression),
+    /// `variable = value` or, when the flag is set, `variable += value`.
+    Entity(String, bool, Expression),
+}
+
 pub struct SetPropertyOperator {
     input: OperatorBox,
-    items: Vec<(String, String, Expression)>, // (variable, property, value_expr)
-    /// Whole-entity assignments: `(variable, merge, value)`. `merge` is `+=`.
-    entity_items: Vec<(String, bool, Expression)>,
+    /// The writes, in the order the query wrote them.
+    ///
+    /// Applied in that order because the order between a property and a
+    /// whole-entity write decides the result: `SET n = {a: 1}, n.k = 7` keeps
+    /// `k`, and `SET n.k = 7, n = {a: 1}` replaces it. Applying every
+    /// property write first and every entity write after made the second form
+    /// right and the first silently lose `k` (#1896).
+    writes: Vec<SetWrite>,
 }
 
 impl SetPropertyOperator {
     pub fn new(input: OperatorBox, items: Vec<(String, String, Expression)>) -> Self {
-        Self { input, items, entity_items: Vec::new() }
+        let writes = items.into_iter().map(|(v, p, e)| SetWrite::Property(v, p, e)).collect();
+        Self { input, writes }
     }
 
-    /// With whole-entity assignments (`SET n = {…}`, `SET n += {…}`).
+    /// The property writes, then the whole-entity ones (`SET n = {…}`,
+    /// `SET n += {…}`). For writes in the order a query gave them, use
+    /// [`SetPropertyOperator::from_writes`].
     pub fn with_entity_items(
         input: OperatorBox,
         items: Vec<(String, String, Expression)>,
         entity_items: Vec<(String, bool, Expression)>,
     ) -> Self {
-        Self { input, items, entity_items }
+        let mut writes: Vec<SetWrite> =
+            items.into_iter().map(|(v, p, e)| SetWrite::Property(v, p, e)).collect();
+        writes.extend(entity_items.into_iter().map(|(v, m, e)| SetWrite::Entity(v, m, e)));
+        Self { input, writes }
+    }
+
+    /// Writes applied in exactly this order.
+    pub fn from_writes(input: OperatorBox, writes: Vec<SetWrite>) -> Self {
+        Self { input, writes }
     }
 
     /// The properties a right-hand side contributes.
@@ -20697,8 +20722,11 @@ impl PhysicalOperator for SetPropertyOperator {
 
     fn next_mut(&mut self, store: &mut GraphStore, tenant_id: &str) -> ExecutionResult<Option<Record>> {
         if let Some(record) = self.input.next_mut(store, tenant_id)? {
-            // Evaluate all SET expressions first (immutable borrow of store)
-            let evaluated: Vec<_> = self.items.iter().map(|(var, prop, expr)| {
+            // Evaluate every property write's value first, before anything is
+            // written, so `SET a.x = a.y, a.y = a.x` swaps. A whole-entity
+            // write is evaluated where it stands, as it always was.
+            let evaluated: Vec<Option<PropertyValue>> = self.writes.iter().map(|w| {
+                let SetWrite::Property(_, _, expr) = w else { return None };
                 let val = match eval_expression(expr, &record, store) {
                     Ok(v) => match v {
                         Value::Property(pv) => pv,
@@ -20748,7 +20776,7 @@ impl PhysicalOperator for SetPropertyOperator {
                     },
                     Err(_) => PropertyValue::Null,
                 };
-                (var.clone(), prop.clone(), val)
+                Some(val)
             }).collect();
 
             // A property is a scalar or a list of scalars. `SET a.maplist =
@@ -20758,7 +20786,8 @@ impl PhysicalOperator for SetPropertyOperator {
             // logic, so the shared `storable_property` never saw it; the check
             // is applied to the result instead, which covers whichever
             // converter produced it.
-            for (_, prop, val) in &evaluated {
+            for (w, val) in self.writes.iter().zip(&evaluated) {
+                let (SetWrite::Property(_, prop, _), Some(val)) = (w, val) else { continue };
                 if !property_is_storable(val) {
                     return Err(ExecutionError::TypeError(format!(
                         "InvalidPropertyType: `{prop}` cannot hold a map inside a list. \
@@ -20767,8 +20796,19 @@ impl PhysicalOperator for SetPropertyOperator {
                 }
             }
 
-            // Apply mutations via store methods (syncs columnar + row + index)
-            for (var, prop, val) in &evaluated {
+            // Apply mutations via store methods (syncs columnar + row + index),
+            // in the order the query wrote them.
+            for (w, val) in self.writes.iter().zip(&evaluated) {
+                let (var, prop, val) = match (w, val) {
+                    (SetWrite::Property(var, prop, _), Some(val)) => (var, prop, val),
+                    (SetWrite::Entity(var, merge, expr), _) => {
+                        let Some(target) = record.get(var).cloned() else { continue };
+                        let value = eval_expression(expr, &record, store)?;
+                        apply_entity_assignment(&target, &value, *merge, store, tenant_id)?;
+                        continue;
+                    }
+                    _ => continue,
+                };
 
                 if let Some(node_val) = record.get(var) {
                     // `SET n.prop = null` **removes** the property. Storing a
@@ -20800,14 +20840,6 @@ impl PhysicalOperator for SetPropertyOperator {
                 }
             }
 
-            // Whole-entity assignment. Evaluated after the per-property items
-            // so that `SET n.a = 1, n = {b: 2}` behaves as written rather than
-            // as ordered by implementation detail.
-            for (var, merge, expr) in &self.entity_items {
-                let Some(target) = record.get(var).cloned() else { continue };
-                let value = eval_expression(expr, &record, store)?;
-                apply_entity_assignment(&target, &value, *merge, store, tenant_id)?;
-            }
 
             Ok(Some(record))
         } else {
@@ -20824,10 +20856,16 @@ impl PhysicalOperator for SetPropertyOperator {
     }
 
     fn describe(&self) -> OperatorDescription {
-        let mut sets: Vec<String> = self.items.iter().map(|(v, p, e)| format!("{}.{} = {}", v, p, format_expression(e))).collect();
-        sets.extend(self.entity_items.iter().map(|(v, merge, e)| {
-            format!("{} {} {}", v, if *merge { "+=" } else { "=" }, format_expression(e))
-        }));
+        let sets: Vec<String> = self
+            .writes
+            .iter()
+            .map(|w| match w {
+                SetWrite::Property(v, p, e) => format!("{}.{} = {}", v, p, format_expression(e)),
+                SetWrite::Entity(v, merge, e) => {
+                    format!("{} {} {}", v, if *merge { "+=" } else { "=" }, format_expression(e))
+                }
+            })
+            .collect();
         OperatorDescription {
             name: "SetProperty".to_string(),
             details: sets.join(", "),

src/query/executor/planner.rs

@@ -1832,16 +1832,12 @@ impl QueryPlanner {
                 // A bare `SET` after MERGE applies on both branches, unlike ON CREATE /
                 // ON MATCH. It parsed but was dropped here, so `MERGE (m) SET m.x = 1`
                 // silently left the property unset.
+                //
+                // Through `attach_set` like every other SET: this path used to
+                // keep only the `n.prop = v` items, so `MERGE (m) SET m = {…}`
+                // and `SET m:Label` were dropped without a word.
                 if !query.set_clauses.is_empty() {
-                    let items: Vec<(String, String, Expression)> = query
-                        .set_clauses
-                        .iter()
-                        .flat_map(|sc| sc.items.iter())
-                        .map(|item| {
-                            (item.variable.clone(), item.property.clone(), item.value.clone())
-                        })
-                        .collect();
-                    operator = Box::new(SetPropertyOperator::new(operator, items));
+                    operator = Self::attach_set(operator, &query.set_clauses);
                 }
 
                 let mut output_columns = Vec::new();
@@ -8800,29 +8796,7 @@ impl QueryPlanner {
                     bound.insert(u.variable.clone());
                 }
                 Clause::Set(sc) => {
-                    let items: Vec<(String, String, Expression)> = sc
-                        .items
-                        .iter()
-                        .map(|i| (i.variable.clone(), i.property.clone(), i.value.clone()))
-                        .collect();
-                    let entity_items: Vec<(String, bool, Expression)> = sc
-                        .entity_items
-                        .iter()
-                        .map(|i| (i.variable.clone(), i.merge, i.value.clone()))
-                        .collect();
-                    if !items.is_empty() || !entity_items.is_empty() {
-                        operator = Box::new(SetPropertyOperator::with_entity_items(
-                            operator, items, entity_items,
-                        ));
-                    }
-                    let adds: Vec<(String, Label)> = sc
-                        .label_items
-                        .iter()
-                        .flat_map(|i| i.labels.iter().map(|l| (i.variable.clone(), l.clone())))
-                        .collect();
-                    if !adds.is_empty() {
-                        operator = Box::new(LabelMutationOperator::new(operator, adds, Vec::new()));
-                    }
+                    operator = Self::attach_set(operator, std::slice::from_ref(sc));
                 }
                 Clause::Remove(rc) => {
                     let mut props = Vec::new();
@@ -9336,31 +9310,38 @@ impl QueryPlanner {
         operator
     }
 
-    /// `SET` over `operator`: property and whole-entity items, then labels.
+    /// `SET` over `operator`, one clause after another.
+    ///
+    /// Each clause gets its own operators, so a later clause's writes land
+    /// after an earlier clause's. Flattening every clause into one list lost
+    /// that, and `SET n = {…} SET n.k = 7` dropped `k` (#1896). Within a
+    /// clause the property and whole-entity writes keep the order they were
+    /// written in; labels follow, which no property write can disturb.
     fn attach_set(mut operator: OperatorBox, set_clauses: &[SetClause]) -> OperatorBox {
-        let mut items = Vec::new();
-        let mut label_adds = Vec::new();
-        let mut entity_items = Vec::new();
+        use crate::query::executor::operator::SetWrite;
         for set_clause in set_clauses {
-            for item in &set_clause.items {
-                items.push((item.variable.clone(), item.property.clone(), item.value.clone()));
-            }
-            for item in &set_clause.label_items {
-                for label in &item.labels {
-                    label_adds.push((item.variable.clone(), label.clone()));
+            let mut writes = Vec::new();
+            let mut label_adds = Vec::new();
+            for op in &set_clause.ops {
+                match op {
+                    SetOp::Property(i) => writes.push(SetWrite::Property(
+                        i.variable.clone(),
+                        i.property.clone(),
+                        i.value.clone(),
+                    )),
+                    SetOp::Entity(i) => {
+                        writes.push(SetWrite::Entity(i.variable.clone(), i.merge, i.value.clone()))
+                    }
+                    SetOp::Labels(i) => label_adds
+                        .extend(i.labels.iter().map(|l| (i.variable.clone(), l.clone()))),
                 }
             }
-            for item in &set_clause.entity_items {
-                entity_items.push((item.variable.clone(), item.merge, item.value.clone()));
+            if !writes.is_empty() {
+                operator = Box::new(SetPropertyOperator::from_writes(operator, writes));
+            }
+            if !label_adds.is_empty() {
+                operator = Box::new(LabelMutationOperator::new(operator, label_adds, Vec::new()));
             }
-        }
-        if !items.is_empty() || !entity_items.is_empty() {
-            operator = Box::new(SetPropertyOperator::with_entity_items(
-                operator, items, entity_items,
-            ));
-        }
-        if !label_adds.is_empty() {
-            operator = Box::new(LabelMutationOperator::new(operator, label_adds, Vec::new()));
         }
         operator
     }