[nareshkolimi]

Fixes #1897.

The bug

Statement	Before	After
MATCH (p:V {name:'Alice'}) SET p.embedding = vecf32([0.1, 0.2])	success, nothing stored	UnknownFunction: vecf32 … at compile time
MATCH (p:V {name:'Alice'}) SET p.x = noSuchFunction(1)	success, nothing stored	UnknownFunction
RETURN vecf32([0.1, 0.2])	UnknownFunction	unchanged

SET n.embedding = vecf32([...]) is how FalkorDB writes a vector, so a migrating user's import reported success while storing no vectors, and compatibility_report counted the statement as accepted.

Two causes, both fixed

1. The compile-time check never saw write clauses. validate_function_names (An unknown function returns zero rows instead of failing #947) walked all_expressions, which covers RETURN, WITH, WHERE, UNWIND and LOAD only. It now also walks a new write_expressions, which covers:

   • SET values, both forms, in by-kind and pipeline queries;
   • ON CREATE / ON MATCH values;
   • DELETE targets;
   • FOREACH lists and bodies, nested ones included.

   It also walks the existing pattern_property_expressions, so CREATE (n {x: noSuchFunction(1)}) is refused too. all_expressions itself is unchanged, because the other checks built on it were measured against reading clauses alone.

2. A failed evaluation became a delete. SetPropertyOperator mapped an evaluation error to Null (Err(_) => PropertyValue::Null), and a null SET removes the property. So an expression that failed at run time reported success and deleted what was there. The error is now the statement's error. The change is four lines, kept small on purpose because A SET applies its writes in the order they were written (#1896) #1916 rewrites the code around it.

Tests

tests/unknown_function_in_writes.rs:

• the two statements from the issue are refused, and the message names the function;
• 11 updating-clause positions (SET =/+=/a second SET clause, MERGE ON CREATE/ON MATCH/bare SET, CREATE properties, FOREACH list and body, a pipeline SET) each refuse an unknown function;
• known functions in those positions (timestamp(), randomUUID(), size, abs, range) and a namespaced one (date.truncate) are still accepted. The An unknown function returns zero rows instead of failing #947 check was deliberately narrow, because over-rejecting cost 345 TCK scenarios the first time;
• a SET whose value fails at run time (1 / (p.x - 5) with x = 5) returns Division by zero and leaves x as it was.

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

Note

This touches the same lines of SetPropertyOperator::next_mut as #1916 (#1896). Whichever merges second needs a small rebase, and I'll do it.

[Kalyanpulikanti-01]

Approving. Two fixes here, and I'd argue the four-line one is the more important of the two — worth saying so because the PR title and the tests both lead with the other.

Err(_) => PropertyValue::Null was the actual data-loss bug

That arm meant any failure to evaluate a SET right-hand side deleted the property, because a null SET removes rather than stores (#874). Not just an unknown function — a division by zero, a type error, an arithmetic overflow, anything. The statement then reported success. So the blast radius was never "FalkorDB's vecf32"; it was every runtime evaluation failure on the write path, and the 1 / (p.x - 5) test is the one that pins the general case rather than the reported instance.

This also means the two fixes are complementary rather than belt-and-braces, which matters for how much the first one has to carry:

• compile-time check — catches un-namespaced unknown names, data-independently, before anything runs.
• the error arm — catches everything else, including every case the first one deliberately lets past.

The compile-time check cannot catch the FalkorDB vector functions, and that's fine

is_known_function allows every dotted name by design — lower.contains('.') — because rejecting namespaced names at compile time turned 345 passing TCK scenarios into errors the first time this was attempted. So vecf32 is caught (no dot), but vector.similarity.cosine(...), algo.* and db.* in a SET still pass validation untouched, before and after this PR.

That isn't a gap in the PR, but it does mean the date.truncate test proves less than it looks like it proves — a dotted name passes trivially, whatever it is. The thing actually protecting a user who writes SET n.v = vector.similarity.cosine(...) is the error arm, which now surfaces it at run time instead of silently emptying the property. Worth a line in the issue or the guide, since #1897 was filed from a FalkorDB migration and vecf32 is the only one of that family the compile-time half reaches.

Over-rejection: the risk is lower than it looks, and for a structural reason

This was my main concern, given #947's history. Checking it:

A function implemented in eval_function but missing from KNOWN_FUNCTIONS would now break a working SET. But such a function is already rejected in RETURN today, so the gap would already be a live bug for reads rather than something this PR introduces. every_accepted_function_can_actually_execute (#1456) pins the list→dispatcher direction; the dispatcher→list direction is pinned only by the first failure in production, and that is pre-existing.

The residual risk specific to this change is narrower: a parser-generated synthetic call that appears only in write positions and doesn't start with __ would now be rejected where nothing checked it before. I don't see one, and the 11 positions tested cover the surface, but that is the class worth watching if something odd turns up.

Since the suite that caught the 345-scenario regression is not in per-PR CI, it is worth saying explicitly whether the TCK was run locally against this. If it was, the result belongs in the PR body; if it wasn't, that is the one check I'd want before merge rather than after.

Scope and the rebase

Keeping the operator change to four lines because #1916 rewrites around it was the right call, and the note about it is accurate — write_expressions reads sc.items and sc.entity_items as fields, which #1916 turns into iterator methods. So whichever lands second needs .items() / .entity_items(), a mechanical change. Given #1916 is also approved, suggest landing #1916 first and rebasing this onto it: #1916's change is the larger of the two and its SetOp enum is what the validator will want to walk eventually anyway.

Worth noting for the release notes that compatibility_report counted these statements as accepted, so the FalkorDB corpus numbers in #1898 — 53 accepted / 38 planned / 32 run — were measured against an engine that accepted a write it then silently discarded. Those figures should be re-derived after both of these land, not carried forward.

[Kalyanpulikanti-01]

I've pushed the dev merge to this branch (2ba67de) since it had gone DIRTY and #1897 is a silent-write-loss bug worth unblocking. Please check my reading of the conflict — it was a judgement call, not the mechanical rebase we both expected.

It wasn't just .items() / .entity_items()

That part was there (validate.rs:2516-2517, now the accessor form). But #1916 also changed SetPropertyOperator::next_mut's value-evaluation closure to return Option<PropertyValue> over the new ordered writes vector — and your fix turns a failed evaluation into the statement's error, so Err(e) => return Err(e) no longer compiles inside a closure returning Option.

Taking either side alone silently drops one of the two fixes, which is the exact failure mode both PRs exist to prevent. So I combined them:

let evaluated: Vec<Option<PropertyValue>> = self.writes.iter().map(|w| {
    let SetWrite::Property(_, _, expr) = w else { return Ok(None) };
    let val = match eval_expression(expr, &record, store) {
        ...
        Err(e) => return Err(e),
    };
    Ok(Some(val))
}).collect::<ExecutionResult<_>>()?;

ExecutionResult<Option<PropertyValue>> per item: None for a non-property write, Err propagating out through the collect. Three properties had to hold at once, and each has a test that fails if I got it wrong:

must hold	from	test
writes apply in source order	#1916	a_property_after_a_replace_is_kept
property values all read before any write (so SET a.x = a.y, a.y = a.x swaps)	#1916	property_values_are_still_read_before_any_write
a failed evaluation errors and writes nothing	#1897	a_set_whose_value_fails_at_run_time_is_an_error_and_writes_nothing

Verified

cargo check --lib --tests clean, no new warnings, none in the regions touched. Then, in the debug profile CI uses:

set_order                  8 passed
unknown_function_in_writes 4 passed
unknown_function          11 passed
merge_sees_its_own_writes  7 passed

30/30. The middle two rows of that table are the ones I actually cared about: property_values_are_still_read_before_any_write passing means I didn't flatten your evaluate-then-write split into evaluate-at-write, and a_set_whose_value_fails_at_run_time_is_an_error_and_writes_nothing passing means the error path survived the restructure. Either resolution that looked right but was wrong would have failed one of them.

dev has moved 3 commits since, so this needs one more update before it can merge. Queuing it.

Shout if you'd rather have resolved it yourself and I'll revert the merge commit — it's one commit on top of yours and nothing else is touched.

[Kalyanpulikanti-01]

CI failed after the dev merge, and the cause is worth recording because it is not a defect in this PR — #1898's corpus ratchet caught it, correctly.

Exactly one idiom changed:

MATCH (p:Person {name: 'Alice'}) SET p.embedding = vecf32([0.1, 0.2, 0.3])

refused now with UnknownFunction: vecf32 …, so the FalkorDB corpus went 38 accepted → 37 and both the_accepted_count_does_not_fall and no_query_that_used_to_be_accepted_is_refused_now fired.

That is the statement from #1897 itself. It was counted as accepted because validation only walked the reading clauses — so it planned, ran, failed to evaluate the call, and reported success having stored nothing. The count fell because the honesty rose, which is the one direction the floor is not meant to guard against.

Pushed as 4629fac:

• the idiom joins KNOWN_REFUSALS with its reason and the issue number;
• ACCEPTED_FLOOR drops to 37 with a comment saying why — a floor lowered silently is indistinguishable from a regression being papered over, and forcing that distinction is the whole point of the ratchet.

every_known_refusal_still_matches_a_corpus_query keeps the new entry honest: reword the corpus query and the entry stops matching, which fails the build rather than quietly covering nothing.

Verified in the debug profile CI uses:

falkordb_compatibility_corpus  5 passed
falkordb_migration_guide       7 passed
set_order                      8 passed
unknown_function_in_writes     4 passed

24/24. falkordb_migration_guide's the_recommended_replacements_run passing matters as much as the ratchet here — it confirms the newly-strict validator does not refuse any of the replacements the migration guide tells users to write instead, which was my main over-rejection worry when I reviewed this.

One thing for whoever lands this: docs/MIGRATING-FROM-FALKORDB.md states 53 accepted / 38 planned / 32 run as measured figures, and the 38 is now stale. I've flagged it on #1898 rather than editing the guide from here, since those numbers came from a real FalkorDB run and should be re-derived rather than decremented by hand.