fix(query): let OPTIONAL extend a row that left the shared variable unbound

[aaj3f]

Fixes #1713.

An OPTIONAL did nothing at all to the rows on which the required side had left the shared variable unbound. On the seed_values_dataset fixture:

SELECT ?s ?f WHERE {
  ?s schema:name ?name .
  { { ?s ex:friend ?f } UNION { ?s schema:age ?age } }
  OPTIONAL { ?s ex:friend ?f }
}

returned 10 rows — byte-identical to the same query with the OPTIONAL deleted outright. §18.2.4 says 13: the UNION emits six solutions binding ?f and four leaving it unbound, and a solution with ?f unbound is compatible with one that binds it, so those four have to be extended rather than passed through.

The UNION is only how I found it. The merge asks whether the required row's column is Binding::Unbound and never asks how the column got that way, so the scope of the fix is a shared variable the required row left unbound, however it got there — and VALUES … UNDEF lowers to exactly that binding (parse/lower.rs:491). bplatz's VALUES-with-UNDEF repro on #1713, which has no UNION anywhere, is broken at the merge base in the same way and closed here by the same one-place change, on both surfaces and on both builder lanes. That's what the tests now pin, and it's why I think #1713 closes as the class rather than as one repro.

Five things had to be true at once, and I think all five are worth naming. The first is the "real" bug; the next three are what made it invisible on one lane and differently wrong on the others; the fifth is a fourth builder with the same defect and a worse symptom, which I found while checking that the first four really did cover every lane.

The merge dropped the value it had just matched. OptionalOperator::combine_rows (fluree-db-query/src/optional.rs) copied every required column verbatim and then appended only the optional-only columns. unify_check right above it already does the right thing — it treats an unbound left value as compatible with any right value — but nothing then read that right value back out, so a row the optional side genuinely matched came out exactly as it went in. It now fills the shared columns the required row left unbound from the optional side, which is just SPARQL merge. The patch is driven off a precomputed shared_merge_cols (the builder's own unify columns), for the canonical OPTIONAL { ?s :email ?e } it holds a single entry (?s), and the cost per output row is one is_unbound test per merge column — the fill scan itself only runs on a column that is actually Unbound.

The result cache keyed on a third of the correlation. PatternOptionalBuilder::cache_key keyed only on the subject, while substitute_pattern substitutes subject, predicate and object. So on the novelty lane the (cam, ?f=alice) row's answer — a one-row existence batch for cam friend alice — was served straight back to the (cam, ?f=unbound) row out of the LRU. That is what made the counts land exactly on the no-OPTIONAL answer, which is a nastier failure mode than being merely wrong: it's the cache making a wrong answer look self-consistent. The key is now the substituted pattern, mirroring substitute_pattern position by position: a slot that substitution pushes a value into keys by that value, and a slot it leaves free gets the same u token whatever the row held, so two rows that would drive the identical scan share one entry (unify_check re-applies the row's own correlation when the pending match drains, so sharing is sound). Concretely that means a late-materialized EncodedSid/EncodedPid/EncodedLit in the object slot now shares one scan instead of driving N identical ones or declining to cache at all. For the ordinary shape — left-bound ?s, free ?o — the key is still just the subject and left-side fan-out reuses right-side results exactly as before.

The batched subject probe read the object off the plan-time template. PatternOptionalBuilder::build_batch probes (subject, predicate) and takes the object slot from self.pattern.o, never from the row — sound while the object variable is optional-only, wrong the moment the required side carries values for it. On an indexed ledger the same query returned 21 rows: one bare existence row per matching triple, so a required row that bound ?f was duplicated once per friend instead of filtered, and a required row that left it unbound was still passed through. build_batch and supports_seed_coalescing now decline when the object variable is shared with the required side, and the shape falls to the per-row substituted scan, which pins the row's own object exactly (and leaves an unbound one free). I went back and forth on instead widening emit_object_var so the probe materializes the object and lets unify_check filter — it's the faster shape — but that would newly rest object correlation on Binding equality across representations decoded from the index vs. produced upstream, and Binding's PartialEq answers false rather than erroring across those pairs, so it would silently drop rows; #1729 is a live filed instance of exactly that on the Lit/dtc arm. For a P1 silent-wrong-results fix I'd rather not take that on. Worth being precise about what declining buys, though, and the code comment now says so: it narrows the exposure rather than removing it, because substitute_pattern still leaves a late-materialized EncodedSid/EncodedPid/EncodedLit object free and unify_check's left_val == right_val is then what enforces the correlation on the per-row path too.

PlanTreeOptionalBuilder::build_batch had the same class of problem one level up. It marked a row with an unbound correlation variable "unmatchable" and emitted a no-match row. A hash partition by correlation key genuinely can't express "compatible with every bucket", and its optional-side batches carry only optional-only vars so there'd be nothing to read the value back off — so that path hands the batch to the per-row lane instead. Scoped, though, to a correlation variable the inner can actually bind. corr_cols is every required column referenced anywhere in the inner patterns, and Pattern::Filter is hash-join safe, so OPTIONAL { ?s ex:friend ?f . FILTER(?age > 20) } with ?age unbound on half the rows would otherwise take the whole (coalesced, up to 512K-row) driving side off the lane while changing nothing — the inner can't bind ?age, so the filter errors under correlated and independent evaluation alike and the old no-match row was already right. The gate is inner_patterns.produced_vars(), the same notion PlanTreeOptionalBuilder::new already uses for optional_vars, plus a small escape hatch: a filter that can still answer true with an unbound operand (BOUND, COALESCE, IF, ||, XOR, EXISTS) keeps the per-row lane, because for those the row genuinely can still join. Everything else propagates the error — && included, since error && true is an error and error && false is false, never true.

GroupedPatternOptionalBuilder was the fourth lane, and it was fabricating solutions. Two-or-more chained single-triple OPTIONALs on one subject route here (execute/where_plan.rs:2411-2426), and its unify_instructions() was hardcoded &[] — so shared_merge_cols was empty for it and combine_rows never patched anything. Its object variables are structurally optional-only (collect_grouped_single_triple_optionals breaks on required_schema.contains(&o_var)), but its subject only has to be present in the required schema, not bound on every row. So:

SELECT ?s ?e ?a WHERE {
  { { ?s schema:name ?n } UNION { ex:nikola schema:name ?nn } }
  OPTIONAL { ?s schema:email ?e }
  OPTIONAL { ?s schema:age ?a } }

answered nine rows, four of them [null, "alice@example.org", 50], [null, "brian@example.org", 50] and so on — the right cardinality, the right ?e/?a correlation, and the subject never written back. That's the fabricated-solution flavour bplatz called the nastier failure mode on #1713: a row asserting an email and an age for a subject it declines to name. It isn't a regression (the merge base answered a worse 21-row ?e×?a cross-product for the same query), but it is exactly the class this change is named for, so it's fixed here rather than deferred. The builder now reports its subject as a merge column, and the build_batch gate that keeps that sound — the batched lane's own schema is optional-only, so it has no subject to read back — is now an explicit unbound-subject decline rather than an accident of resolve_subject_id returning None. The per-row chain's output does carry the subject at subject_left_col, which is where the merge picks it up.

The remaining door: #1734

Under that sharper framing one ordinary shape is still wrong, and it is not fixed here: two OPTIONALs on the same variable.

?s schema:name ?name . OPTIONAL { ?s ex:greeting ?f } OPTIONAL { ?s ex:friend ?f }

answers 6 rows where §18.2.4 wants 9 — ex:alice/ex:brian/ex:cam/ex:liam all come back [?s, null], so the friends are found and then dropped and the fan-out goes with them. Give the two OPTIONALs distinct variables and the same fixture answers the correct 9. The reason is that a preceding OPTIONAL that matched nothing records Binding::Poisoned, not Unbound (create_poisoned_row, optional.rs), unify_check refuses poisoned pairings, and this merge deliberately skips a column that isn't Unbound. Poison-blocks-matching is a deliberate, documented engine semantic (binding.rs:48-53), so closing that door means deciding whether the blocking behaviour is right at all — which is a design call I don't think belongs inside a P1 silent-wrong-results fix. Filed as #1734 (P1, triage:needs-decision); it is byte-identical at the merge base and here, so nothing in this PR moved it. Closing #1713 on the Unbound doors and naming #1734 as the Poisoned one seemed more honest than either claiming the whole class or leaving the gap implied.

Performance

The PlanTree fallback is the only hunk that can cost anything, and the scoping above is what keeps it off the rows it buys nothing for. Measured on an IC5-shaped fixture (300 forums × 25 members × 6 posts, a 300-row VALUES driving a two-pattern correlated OPTIONAL with a FILTER on a VALUES-supplied ?age, 7,500-row driving side, debug, same box and process):

	all correlation bound	one UNDEF ?age cell
scoped to what the inner can bind (this PR)	50.8 s	50.6 s
bail on any unbound correlation column	46.7 s	555.2 s

One unbound filter operand in 300 driving rows was an 11.9× cliff, and every row it slowed down was already answering correctly before and after. The blast radius really is the driving side rather than a batch — supports_seed_coalescing can't see data, so the operator coalesces first and build_batch declines after. Putting the UNDEF at forum 150 of 300 landed roughly half the 7,500 rows on the per-row path, which is the ratio you'd expect against FLUREE_OPTIONAL_HASH_JOIN=0 (that run came in at ~1,060 s, but it overlapped another build on this box so treat the ratio rather than the absolute as the signal). I confirmed the answers are identical either way — the new optional_reading_an_unbound_filter_operand_pads_the_row test passes with the scoping and with it forced off — and that the lane really does stay on, via the operator's own debug counters (batched_builds=1, built_optionals=0 with the scoping; built_optionals non-zero plus a split batch without it).

Memory is neutral: shared_merge_cols is one Vec<(usize, VarId)> per operator with no per-row allocation, and the combine_rows position scan only runs for a column that is actually Unbound.

One cost I did not remove: a correlated literal object (?s :p ?o . OPTIONAL { ?s :q ?o } with ?o a literal) now goes uncached, because substitution genuinely pushes that literal down and two rows carrying different literals must not share an entry. Keying it would need a stable byte encoding of every FlakeValue/datatype pair, which is more than this fix should be buying; the comment at the decline says so. It's wrong-but-cached → right-but-uncached, so nothing regressed against a correct baseline, but it is a real per-row cost on that one shape.

Bench

Nothing in regression-budget.json covered OptionalOperator at all, which means CI could not have caught any of the five defects above — and the fallback lanes are an order of magnitude slower than the batched probes they replace, so this is a path where a scoping change is worth gating. fluree-db-api/benches/query_hot_optional.rs adds one scenario per lane — the batched subject probe, the object-correlated per-row scan (where a cache-hit-rate change shows up), the PlanTree batched hash join, and the unbound-filter-operand shape — on an indexed file-backed ledger, with the same build-once/reindex/warm-snapshot discipline as query_hot_property_path.rs. Budget entry at tiny 10% / small 5% / medium 3%, matching the other query_hot_* benches; cargo test -p fluree-bench-support --test workspace_reconcile is green.

The fourth scenario is a real sentinel rather than decoration: at tiny scale (200 persons, release) it runs in 382 µs, and with the correlation scoping forced back to every correlation column it runs in 6.01 ms — 15.7×, far outside any budget.

Tests

fluree-db-api/tests/it_optional_after_union.rs, its own binary because the indexed case needs the background indexer. Ten tests:

• the SPARQL query above and its JSON-LD twin (["union", …] then ["optional", …]) on novelty, and the same on an indexed view;
• a two-pattern OPTIONAL that routes to PlanTreeOptionalBuilder instead;
• one where the UNION's second branch binds neither correlation variable, so the left join has to extend a completely free row;
• the VALUES … UNDEF door from bplatz's comment on OPTIONAL after a UNION is a no-op over rows the UNION left unbound (deleting the clause returns identical rows) #1713 — SPARQL, the JSON-LD twin, and a multi-pattern variant on the PlanTree lane. All three return the eight all-null rows he reported at the merge base and the correct eight here, matching the no-VALUES baseline exactly (the multi-pattern one returned five, all null, so the correlation-key partition was losing multiplicity as well as the binding);
• the grouped-lane chain, asserting both the exact nine rows and that no solution reports an email/age for a subject it leaves unbound. Pinned on novelty only: the grouped batched probe declines an unbound-subject batch outright, so both lanes reach the same per-row chain and an indexed twin would be a duplicate. I did check it by hand rather than assume — on an indexed view (binary_store().is_some()) the same query returns the same nine rows with ?s bound on every one;
• the filter-only correlation shape, pinning the ten rows the scoping must preserve.

Plus unit tests in optional.rs for the grouped builder's merge column, the bindable-vs-read-only correlation split, the filter strictness classification (?age > 20 and a && b strict; !BOUND(?age) and a || true not), and the cache key — that an object slot substitution leaves free keys identically to Unbound while a subject slot keys by value.

Each integration test asserts the exact row multiset and a property that can't be satisfied by accident: for the UNION form, that deleting the OPTIONAL changes the answer; for the UNDEF form, that the answer equals the no-VALUES baseline. A plan-shape or row-count check would have gone green on a half fix — the free-row case was already emitting the right number of rows before this change, just with (null, null) in every one of them.

Non-vacuity, per the repo's habit: reverting the combine_rows merge alone turns nine of the ten red — the tenth is the filter-operand test, which pins the scoping rather than the merge and correctly stays green. Reverting only the grouped builder's unify_instructions turns exactly the grouped test red and leaves the other nine green.

W3C registers

testsuite-sparql/tests/registers/mod.rs:143-144 registers algebra/manifest#nested-opt-1 and #nested-opt-2 under "correlated-OPTIONAL independence", and #1713 asked whether they'd flip. They don't, and I'm fairly confident that's correct rather than a half fix: they're about where the right operand is evaluated, not how it merges. nested-opt-1 wants { :x3 :q ?w . OPTIONAL { :x2 :p ?v } } evaluated independently, binding ?v=2 on its own and so matching nothing against ?v=1 — one solution. Every OptionalBuilder here instead seeds the inner subplan from the required row, so ?v=1 gets substituted, the inner OPTIONAL finds nothing, ?w=3/4 passes through, and we answer two. That's a genuinely separate defect from the unbound-merge one.

I checked rather than assumed: both tests' actual output is byte-identical before and after this change (unregistered them, ran sparql10_query_eval_tests on both sides, diffed). They stay registered, and I extended the register comment with the concrete mechanism so the next person doesn't have to re-derive it. Full --test w3c_sparql is green — 36/36 — with the register enforced both ways, which is also the evidence that nothing else in the suite moved.

Follow-up

• Two OPTIONALs on the same variable: the second loses its results and fan-out when the first matched nothing (Poisoned blocks the merge) #1734 — the Poisoned door above. Needs a decision on poison-blocks-matching, not a patch.
• fix(query): keep a VALUES written after an OPTIONAL below it #1701 is still open. Its test a_union_binding_the_var_does_not_suppress_the_barrier asserts plan shape rather than rows precisely because pinning rows there would have pinned this defect's output — once both land that should be upgraded to a row assertion, which is then the natural regression check for both issues at once.

Gates

cargo test -p fluree-db-api green (full sweep — 43 targets, 3,269 tests, not just the query groups); -p fluree-db-query green; -p fluree-db-cypher -p fluree-db-sparql -p fluree-db-reasoner green; cargo test -p fluree-bench-support --test workspace_reconcile green; cargo bench -p fluree-db-api --bench query_hot_optional -- --test green; testsuite-sparql --test w3c_sparql 36/36; cargo clippy --all-targets --no-deps -D warnings clean on both touched crates and in testsuite-sparql; cargo fmt --all last, in the workspace and in testsuite-sparql/.

CI on this head: fmt, clippy, testsuite-sparql, bench-paths, bench-compare and plan all pass. test reports exactly one failure, it_ledger_lifecycle::ledger_exists_on_file_storage — inherited from main (43d758610 in #1716 changed the semantics and its test wasn't updated), and nothing in this branch touches that path.

[bplatz]

Approving. Two small items inline, neither blocking.

I'd already verified this defect on pristine main when filing the VALUES … UNDEF comment on #1713, so I re-ran both doors here rather than trusting the description:

	merge base	this branch
VALUES (?s ?f) { (… UNDEF) } + OPTIONAL	8 rows, ?f Null on every one	8 rows, exactly equal to the no-VALUES baseline
the UNION repro	10 rows = OPTIONAL-deleted	13 rows, no longer identical

The first is the convincing one — ex:alice → ex:brian, ex:cam → ex:alice/ex:brian, ex:liam → ex:alice/ex:brian/ex:cam, Null only where there is genuinely no friend. That's the merge happening, not the row count coincidentally landing. Confirmed #1734's door is still open here (6 rows, alice/brian/cam/liam Null) and agree it's a different mechanism that doesn't belong in a P1 wrong-results fix; I did not independently check the byte-identical-at-merge-base claim for it.

Performance — no meaningful risk, and the one real hazard is the one you measured. Checked rather than assumed:

• cache_key: diffed old against new. Common shape was Vec::with_capacity(9) writing 9 bytes, now Vec::with_capacity(16) writing 10 (one extra position tag) — same allocation count per row. Net win elsewhere, since a free-object correlation was previously uncached entirely and now shares one scan across N rows. The literal-object decline was wrong-but-cached before, so nothing correct got slower.
• The build_batch decline is narrowly scoped (Term::Var shared with the required side) — exactly the shape that returned 21 rows on an indexed ledger. Ordinary OPTIONAL { ?s :p ?o } keeps the batched lane, so no correct-and-fast case moved.
• The PlanTree scoping to inner_patterns.produced_vars() is the right narrowing and the 50.6 s / 555.2 s table is the right measurement.

The bench work is the most durable part of this and worth saying so explicitly: regression-budget.json had no OptionalOperator coverage at all, so none of these five defects could have been caught by CI, and the fallback lanes are an order of magnitude slower than the probes they replace. 382 µs vs 6.01 ms is a real sentinel, not decoration.

[bplatz]

The description says shared_merge_cols "is empty for the overwhelmingly common OPTIONAL that shares only already-bound correlation vars." It isn't.

build_unify_instructions (:256) takes the pattern's produced vars, drops the optional-only ones, and keeps whatever remains in the required schema — so for the canonical OPTIONAL { ?s :email ?e }, ?s yields a UnifyInstruction and this vec has one entry.

Doesn't change the conclusion: one matches! per output row against a function that already allocates a Vec<Binding> and clones every binding is nothing. But the sentence claims a stronger property than the code delivers, and it's the kind of line that gets quoted in a later review. "One entry for the canonical shape; the position scan runs only on a column that is actually Unbound" is both true and still a good answer.

[bplatz]

Suggest adding a line to #1734 noting that this merge makes the Unbound/Poisoned split observable rather than theoretical.

Before this change neither door worked, so the behaviour was uniformly wrong. Now it's selectively correct depending on how the null arose — and both render as null in output. Two rows that look identical in a result set behave differently in the next OPTIONAL.

Right call to keep it out of this PR. Worth recording on the issue so whoever picks it up knows the cost of leaving it, since that's the argument for its priority.

[aaj3f]

Thanks @bplatz — both landed, and an apology for the slow close-out on this one: I'd corrected the description when your review came in but hadn't replied here, and on a second look your first comment was sharper than I initially credited.

The shared_merge_cols claim. You were right, and it lived in two places — the PR description, and a doc comment on the field itself at optional.rs:2297, which is exactly the "gets quoted in a later review" hazard you named. Both now carry the true statement (c760aecca for the code side): for the canonical OPTIONAL { ?s :email ?e } the vec holds one entry (?s), the per-row cost is one is_unbound test per merge column, and the fill scan itself only runs on a column that is actually Unbound. Your reading of build_unify_instructions was exactly it — produced vars minus optional-only, intersected with the required schema, so the canonical shape's subject survives into the vec.

The #1734 note. Posted there essentially as you framed it: before this PR neither door worked, so the behavior was uniformly wrong; now it's selectively correct depending on how the null arose, both cases render as null, and two identical-looking rows behave differently in the next OPTIONAL. That observability argument is now the stated case for that issue's priority — you were right that it belongs on the issue rather than in a PR thread that closes.

And thank you for the performance verification — the cache_key before/after diff, confirming the build_batch decline is scoped to exactly the shape that returned 21 rows, and the point that the bench work is the durable part (no OptionalOperator coverage existed, so none of the five defects could ever have been caught by CI) is the sentence I'd want quoted six months from now.

History note so the branch reads clearly: the two commits since your review are an empty CI-refresh commit (working around a stale merge-ref issue after main advanced) and the doc-comment fix above — no behavior changes.