[nareshkolimi]

Fixes #1919.

Update after review (2026-10-21)

• The two behaviour changes are now pinned (commit 9401e1a), as requested:
  • on_match_set_to_null_removes_the_property: ON MATCH SET n.k = null removes k. It checks keys(n) as well as n.k, since a stored null and a removed property both read as null.
  • on_create_set_writes_to_a_variable_bound_before_the_merge: MATCH (a) MERGE (n …) ON CREATE SET a.x = 1 writes a.x, once; a second run that matches does not run ON CREATE.
• dev merged in (89607b7). A SET applies its writes in the order they were written (#1896) #1916 has landed, so this PR now shows only its own change. That merge hit one conflict that git did not flag: SET with an unknown function succeeds and writes nothing; in RETURN the same call is refused #1897's write_expressions in validate.rs read MergeClause's old by-kind fields, which this PR replaced with ordered accessors. The text merged cleanly but did not compile; it now uses on_create_set() and the other three accessors.
• Not yet observed failing on unpatched dev. Tests are left to CI here.

For the release notes

A line of its own, not inside "SET ordering":

MATCH (a) MERGE (n …) ON CREATE SET a.x = … now writes to a (#1919). The write was previously ignored without an error. An import that ran this shape had silently skipped those writes, and will now apply them. ON MATCH SET n.k = null now also removes k, as SET n.k = null does.

The bug

The MERGE half of #1896. MergeClause kept its set items in six vectors by kind: property, whole-entity and label, for each of ON CREATE and ON MATCH. Both MERGE operators then applied every n.k = v before every n = {…}. So the upsert MERGE exists for:

MERGE (n:Person {email: row.email})
  ON CREATE SET n = row.props, n.source_id = row.id

stored no source_id, and every later MATCH (n {source_id: …}) found nothing. The issue asked for the symptom to be confirmed by a run. I have not run it locally, because tests run in CI here. The replace-then-property tests below exercise exactly that shape, and the operator code shows the cause directly: property items were applied before entity items.

The fix

As the issue proposes, one ordered Vec<SetOp> per block, applied in order:

• AST (src/query/ast.rs):
  • MergeClause { pattern, on_create: Vec<SetOp>, on_match: Vec<SetOp> }.
  • on_create_set(), on_create_entity_set(), on_create_labels() and the on_match_* equivalents are filtered iterators for readers that only care about one kind (validation, substitution, parser tests).
  • The filters are shared with SetClause.
• Parser: one parse_on_set_block serves both MERGE parse paths, so they can no longer drift. A second ON CREATE SET block appends after the first.
• Operators (src/query/executor/operator.rs):
  • MergeOperator and MatchMergeEdgeOperator each hold a Vec<SetWrite> per block and apply it through a single apply_writes.
  • That replaces four hand-written copies of the property loop and four of the entity loop.
  • with_ordered_sets sets the writes. new and with_entity_sets keep their signatures, so existing callers and coverage tests compile unchanged.
  • EXPLAIN prints the writes in order, through a format_set_write shared with SetPropertyOperator.
• Planner: the three places that plan a MERGE (the MERGE-only shortcut, the pipeline Clause::Merge, and attach_merge for both the node and the relationship operator) build their writes with merge_writes / merge_labels.

On the issue's question of sharing one applier with SetPropertyOperator: I kept two, because the semantics differ for a reason. SetPropertyOperator evaluates every property value before writing, so SET a.x = a.y, a.y = a.x swaps. MERGE's set items have always been evaluated one at a time, after the writes before them. The two MERGE operators now share SetWrite and the formatter rather than the loop.

Two behaviours that now line up

The single-node MERGE path had its own copy of the property loop, and it differed from every other path in two ways:

1. ON MATCH SET n.k = null did nothing there. It now removes k, as ON CREATE, the multi-node path and plain SET already did (ON CREATE SET n = {...} is silently discarded, and SET n.p = null does not remove the property #874).
2. A property write naming another bound variable (MATCH (a) MERGE (n …) ON CREATE SET a.x = 1) was ignored there. It is now applied, as whole-entity writes on that path already were.

Tests

tests/merge_set_order.rs mirrors tests/set_order.rs. Each case also checks that a property the replace should remove is gone:

• ON CREATE, both orders;
• ON MATCH, both orders, over a node with a pre-existing property;
• ON MATCH over two matches, where both are updated in order;
• the UNWIND … MERGE … ON CREATE SET n = row.props, n.k = row.id bulk-upsert shape, read back by MATCH (n {k: 7});
• the pipeline MERGE path, i.e. a second MERGE in the statement;
• the relationship MERGE path (MatchMergeEdgeOperator), ON CREATE and both orders of ON MATCH;
• += with a label.

cargo check --workspace --tests --examples --benches is clean, with no new warnings, and the lint ratchet is under its ceiling.

[Kalyanpulikanti-01]

The ordering fix is right and I'd merge it as-is. I'm requesting changes for one specific, small thing: the two behaviour changes you found have no tests.

What's excellent

Consolidating eight hand-written loops — four property, four entity, across MergeOperator and MatchMergeEdgeOperator — into one apply_writes is more than #1919 asked for and is the reason the two divergences surfaced at all. One parse_on_set_block serving both MERGE parse paths "so they can no longer drift" is the right instinct; six by-kind vectors becoming two ordered ones is exactly the shape.

And keeping two appliers rather than one was the correct call, with the correct reason: SetPropertyOperator evaluates every property value before writing so SET a.x = a.y, a.y = a.x swaps, while MERGE's items have always been evaluated one at a time after preceding writes. Sharing SetWrite and format_set_write but not the loop preserves a real semantic difference instead of flattening it. I asked in the issue whether they should share an applier — this is a better answer than the question.

Stating "review only the second commit, 3785932" is also the right way to post a stacked PR. #1916 has now merged, so an update should shrink this to its own change.

The change request

tests/merge_set_order.rs has nine tests and all nine are about ordering. Neither of the two behaviours in "Two behaviours that now line up" is pinned:

1. ON MATCH SET n.k = null now removes k on the single-node path, where it previously did nothing.
2. MATCH (a) MERGE (n …) ON CREATE SET a.x = 1 now applies, where it was previously ignored.

Both are behaviour changes beyond ordering, both are correct, and both are currently load-bearing on nothing. If someone refactors apply_writes next month, the ordering tests keep them honest and these two silently revert — and #2 in particular reverts to ignoring a write the user asked for, which is the same silent-write-loss class as #1896 and #1897. Two tests, maybe fifteen lines.

I'd also note #2 is a behaviour change users can observe: a write that was being dropped now lands. That's a fix, but it belongs in the release notes as its own line rather than inside "SET ordering", because someone's import may have been quietly relying on the old behaviour — more likely, has a bug they blamed on something else.

One smaller point

The issue asked for the symptom to be confirmed by a run. I have not run it locally, because tests run in CI here.

That's fine and the honesty is appreciated — but the confirmation the issue wanted is available for free: on_create_keeps_a_property_written_after_a_replace failing on dev before the fix is the confirmed run, and stronger than a manual one because it stays. If you ran the new tests against unpatched dev and watched them fail, say so in the body; if not, it's worth doing once, because a regression test never observed to fail is a test whose subject you're inferring. (That's bitten me in this repo — I shipped a gate test that passed for the wrong reason and only its own guard caught it.)

Happy to re-approve as soon as the two tests are in. Nothing else here needs changing.