fix: fail closed on unencodable datatype IRIs; keep the membership-join lane join-equivalent on list rows - #1746
Conversation
…IRIs A typed literal whose datatype IRI has no registered namespace on the ledger used to fall back to xsd:string at both term-identity sites in SPARQL lowering (the triple-pattern constraint and the term_to_binding VALUES twin), so "a"^^ex:NoSuchType matched every plain-string "a" row. Ingest registers every namespace it stores, so no stored term can carry such a datatype — the provably correct answer is the empty result. Resolve the datatype through the non-strict encoder instead: an unregistered namespace yields the EMPTY-namespace full-IRI Sid, a constraint no stored row carries, so the pattern matches nothing in every enforcement lane (dt_compatible requires exact Sid equality outside the XSD numeric families). This is the form the JSON-LD query surface has used all along (parse/lower.rs), which also closes a live SPARQL-vs-JSON-LD answer divergence on the same question. Registered custom datatypes are unaffected: their query-time encode resolves to the same canonical Sid ingest stored. Pinned by unit test at the lowering layer and integration guards on both surfaces, both lanes (novelty + indexed), with matching controls. Fixes #1686
MembershipJoinOperator answers a right triple that binds nothing new as a hash keep/drop, on the premise that a fully-ground triple matches at most once. RDF list rows break that premise inside a single graph — the same (s, p, o) recurs at multiple o_i positions — so above the 256-row driving gate the lane answered a var-object join over list elements as a semi-join while the generic pipeline answers it as a bag join (3 vs 5 on the #1652 fixture): a scale-dependent silent divergence from the lane's own documented join-equivalence invariant. Stats carry no list-ness, so the shape is not plan-detectable. Instead the one-shot drain that builds the key set doubles as the detector: a key tuple inserted twice IS the premise's counterexample (two flakes matched one ground triple). When that happens the operator discards the hash path and routes every row through its existing exact per-row fallback, so the lane's answer equals the generic pipeline's by construction — without ruling on what a var-object join over list positions should mean (that semantics call stays open on #1687). The cost on ordinary shapes is one bool OR per drained key; the decline is surgical, pinned by a MustFire-style stamp assertion on a same-size non-list fixture. Also closes the lane's testability gap: membership_join.rs now stamps the repo's fast-path routing outcome (site membership-join — Proceed when the hash path serves, GateDeclined on the duplicate-key or multi-graph fallback) and the planner honors FLUREE_DISABLE_QUERY_FAST_PATHS for the lane, so differential harnesses can finally express engagement and compare it against the generic join. New own-binary integration test pins both directions above the gate, with span-captured stamps so neither case can silently degrade into a below-gate run.
aaj3f
added
bug
bplatz
left a comment
bplatz
left a comment
There was a problem hiding this comment.
Three stale doc comments the new fallback route leaves behind — two inline, plus one outside the diff:
fast_path_outcome.rs stamp_fast_path says stamps are "called at plan time from the kill-switch gate sites and at open() time from FastPathOperator". This lane adds a third timing — mid-execution, from build_key_set on the first fully-bound row. Worth a clause so the next site author knows that's allowed.
Separately, filed #1765 for the third site on this seam: ComparableValue::to_binding (eval/value.rs) resolves a STRDT datatype with encode_iri_strict and raises Unknown datatype IRI when the namespace isn't registered — BIND(STRDT(?v, <http://no-such-ns.example.com/T>) AS ?x) fails the whole query, while the registered control works. Out of scope here; noting it so the seam is fully mapped.
| self.per_row_join(ctx, &input_batch, row_idx, &mut columns) | ||
| .await?; | ||
| } | ||
| self.fallback_rows += 1; |
There was a problem hiding this comment.
estimated_rows() (:371) still claims "A filter: at most the child's cardinality (ground rows match ≤ once)". This route is exactly where that stops holding — the new fixture returns 532 rows from 296 driving rows.
There was a problem hiding this comment.
Right — verified the numbers against the fixture (it_membership_join_list_rows asserts the 532-from-296 shape and is green at head) and rewrote the comment to describe both regimes. I also chased whether the value now misleads anything, since doc-patching a live planner input would be the wrong fix: the consumers are describe()/EXPLAIN (operator.rs:149) and two runtime lane gates that read a child's estimate — SubqueryOperator's materialize-eligibility check (subquery.rs:237) and the annotation-edge probe's driving-size gate (default_graph_source.rs:204). Both treat a low value conservatively: they decline their optimization and stay on the exact generic lane, so an under-estimate can cost a missed optimization but not correctness. And the operator can't honestly do better — the regime isn't known until build_key_set sees the first batch, while describe() and both gates read the estimate at plan/open time. So the child estimate stays, now documented as the drain lane's upper bound and the fallback regime's potential under-estimate. Fixed in 14d1e83.
| } | ||
| self.key_set = Some(set); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
per_row_join's doc just below still reads "Exact join fallback for a row that does NOT fully ground the pattern" — after this change it's also the path every fully-ground row takes once the drain declines.
There was a problem hiding this comment.
Fixed in 14d1e83 — it now reads as the exact join for any row the keep/drop drain can't answer: one that doesn't fully ground the pattern, or every row once exact_only is set.
bplatz
left a comment
bplatz
left a comment
There was a problem hiding this comment.
One question on the fail-closed choice — inline. Worth looking into before merge, and a follow-up issue is probably the right home for it if it confirms.
| /// matches nothing, which is the spec answer for a term that does not | ||
| /// exist. (The previous `xsd:string` fallback made | ||
| /// `"a"^^ex:NoSuchType` match every plain-string `"a"` row — #1686.) | ||
| fn datatype_sid(&self, dt_iri: &str) -> Sid { |
There was a problem hiding this comment.
Does this premise survive a multi-ledger dataset query? "Ingest registers every namespace it stores" is true of one ledger, and this Sid is resolved once against the lowering snapshot.
map_pattern_to_match (binary_scan.rs:1796-1830) re-encodes the subject, predicate and object Sids into the per-graph snapshot, but not pattern.dtc — it goes to dt_compatible as the lowering snapshot encoded it. So for a datatype registered in ledger B but not in the lowering snapshot A, the constraint is now the EMPTY-ns Sid and matches nothing in B, where before it was xsd:string and matched the wrong rows. Both wrong, but the new one silently drops real matches instead of adding false ones.
The underlying gap (dtc never re-encoded per graph) predates this PR — registered custom datatypes are already broken cross-ledger by it — and I read this off the code rather than building a fixture, so it may well not be reachable in practice. Worth confirming either way; if it holds, a follow-up issue for re-encoding dtc alongside s/p/o would cover both cases.
There was a problem hiding this comment.
You're right, and it's reachable — I built the fixture rather than trust either of our readings. Two memory ledgers through the public query_dataset surface: A (primary) holds a plain-string "a" row, B holds ex:s2 ex:p "a"^^ct:T with ct: registered only on B (code 14). SELECT ?s WHERE { ?s ex:p "a"^^ct:T } over [A, B]:
- at this PR's head:
[]— the silent drop you predicted (B alone as primary returnss2, so the rows are there and the union reaches them); - with main's
xsd:stringfallback swapped back intodatatype_sid:[["ex:s1"]]— the primary's plain-string row, i.e. wrong rows.
Your pre-existing half confirmed too, with a second case: ct: registered on both ledgers but under divergent codes (15 vs 14, via a padded namespace table on the primary) drops the non-primary row identically at head and on main. Full fixture + verbatim output are in #1771, filed for the re-encode-dtc-alongside-s/p/o fix covering both shapes. I looked at folding it in here: it's three dt_compatible sites (binary_scan.rs:846, matches_datatype_constraint at :1286, range.rs:289) plus threading a per-open effective dtc through the scan, which felt past the fold-in line for this PR — but happy to talk it through if you'd rather see it land here.
One point in the new behavior's favor beyond fail-closed-vs-wrong-rows: the EMPTY-namespace Sid carries the full datatype IRI in its name, so #1771's per-graph re-encode can recover the graph-local Sid from it directly — the xsd:string fallback destroyed that information at lowering time, so no downstream fix could ever have repaired the unregistered-on-primary shape on top of it. Kept the PR's behavior and scoped the body's "provably the only correct one" claim to single-ledger, with the multi-ledger caveat under Not in this PR. The fixture also turned up the decode-side sibling on the same surface (to_jsonld against the primary snapshot either errors on B's codes or silently decodes the wrong IRI) — noted in #1771 next to #1639 rather than filed separately.
…k route Review follow-ups on #1746 (bplatz): - membership_join::estimated_rows: the "ground rows match <= once" claim only holds for the drain lane; the per-row-join fallback is a bag join that can exceed the child's cardinality (532 rows from 296 driving rows on the #1687 fixture). Document both regimes and why the child estimate stays the honest hint (the regime is unknown at plan/open time, and the gates that read the value decline optimizations conservatively). - membership_join::per_row_join: also the path every fully-ground row takes once the drain declines (exact_only), not just non-ground rows. - fast_path_outcome::stamp_fast_path: record the third stamp timing this PR added (mid-execution, from build_key_set on the first fully-bound row) so the next site author knows it's allowed.
|
Thanks @bplatz — all three doc notes were right; fixed in 14d1e83. That includes On your inline multi-ledger question: confirmed reachable by fixture, both failure modes as you read them — details in the thread, follow-up filed as #1771. And thanks for filing #1765 — agreed it's the third site on this seam and the only one that fails hard, and the non-strict-encoder direction you sketched there lines up with what the two lowering sites now do. Between this PR (constraint + VALUES lowering), #1765 (expression materialization), and #1771 (per-graph re-encode), I think the seam is now fully mapped. |
2 participants
Fixes #1686. Partially addresses #1687 — the join-equivalence half; the list-position semantics decision stays open there (details below).
Both of these came out of the same review pass over #1674/#1676 and sit on the same literal-identity seam — what a constant literal's datatype means when it meets the scan — so they ship together.
Problem
1. Unencodable datatype IRIs failed open (#1686). Both term-identity sites in SPARQL lowering — the triple-pattern constraint in
lower_literal_with_constraintand itsterm_to_bindingVALUES twin (fluree-db-sparql/src/lower/term.rs) — resolved a typed literal's datatype withencode_iri_strictand fell back toxsd:stringwhen the namespace wasn't registered. So?s ex:p "a"^^ex:NoSuchTypematched every plain-string"a"row, and the VALUES form did the same.encode_iri_strictreturningNonemeans exactly "this ledger has no such namespace", and ingest registers every namespace it stores — so no stored term on that ledger can carry that datatype, and for a single-ledger query the empty result isn't just the spec answer, it's provably the only correct one. (In a multi-ledger dataset the constraint is resolved against the primary ledger only and a non-primary ledger can carry the datatype — that seam predates this PR and is #1771; see Not in this PR.) The JSON-LD surface already gave it (itslower_triple_patternuses the non-strict encoder), which means the two query surfaces disagreed on the same question — verified live before the fix: SPARQL 1 row, JSON-LD 0.2. The membership-join lane silently diverged from the generic join on list rows (#1687).
MembershipJoinOperatoranswers a right triple that binds nothing new as a hash keep/drop, on the premise that a fully-ground triple matches at most once. RDF list rows break that premise inside a single graph — the same(s, p, o)recurs at multipleo_ipositions — and nothing inmembership_join_key_varsexcluded them, so above the 256-row driving gate the lane answered a var-object join over list elements as a semi-join while the generic pipeline answers a bag join. Reproduced above the gate in the new fixture: 276 rows from the lane vs 532 from the generic pipeline on identical data — a scale-dependent silent wrong answer, and a break of the lane's own documented "join-equivalent unconditionally" invariant. On top of that the lane was untestable: nostamp_fast_pathsite, andFLUREE_DISABLE_QUERY_FAST_PATHSdidn't reach it, soMustFire/MustNotFirecouldn't express engagement at all.Changes
Commit 1 — fail closed on unencodable datatypes (
fluree-db-sparql)datatype_sidhelper: non-strict encode, so an unregistered namespace yields the EMPTY-namespace full-IRI Sid — a constraint no stored row carries. Every enforcement lane compares viadt_compatible(exact Sid equality outside the XSD numeric families:range.rs:289novelty,binary_scan.rs:846overlay,binary_scan.rs:1287indexed), so the pattern matches nothing in all of them, and the SPARQL answer now equals the JSON-LD one by construction.it_literal_identity.rspin the empty result and the controls on both surfaces, in both lanes (novelty + indexed).Commit 2 — keep the membership lane join-equivalent (
fluree-db-query)membership_join.rsnow stamps the repo-standard fast-path outcome (sitemembership-join—Proceedwhen the hash path serves,GateDeclinedon the duplicate-key or multi-graph fallback), and the planner honorsFLUREE_DISABLE_QUERY_FAST_PATHSfor the lane (stampingKillSwitch, checked at the choose site somembership_join_key_varsstays a pure predicate). The differential harness can finally compare this lane against the generic join.it_membership_join_list_rowsruns both cases above the gate (296/280 driving rows) with span-captured stamps, so neither case can silently degrade into a below-gate run and pass vacuously; phase 2 re-answers both under the kill switch and requires identical rows. The stale "the lane's answer at or above the gate is unpinned" comment init_fastpath_1652_regression.rsnow points at the new pin.Both fixes were reverted independently during development to confirm their tests fail for the right reason: the #1686 guards reproduce the 1-row fail-open match, and the #1687 fixture reproduces the 276-vs-532 divergence with the lane engaged.
Behaviour changes (release notes)
xsd:stringrows), in triple patterns and VALUES alike. Registered custom datatypes are unaffected.FLUREE_DISABLE_QUERY_FAST_PATHSnow also disables the membership-join lane.Not in this PR
term_to_bindingspecial-casesfluree:embeddingVector→Sid(FLUREE_DB, "vector")before encoding while the triple-pattern site doesn't; that asymmetry predates this PR and is orthogonal to the fail-open bug, so I left it alone rather than touch vector matching without a vector rig.query_datasetlowers against the first default graph's snapshot, and per-graph execution re-encodess/p/oSids but notpattern.dtc(binary_scan.rsbuild_match_val_for_snapshot), so a datatype registered only on a non-primary ledger constrains as the primary encoded it — confirmed by fixture: this PR turns that from matching the primary's plain-string rows (wrong rows) into matching nothing (silent drop), while registered-on-both datatypes with divergent namespace codes were already dropped before it. The re-encode fix belongs with the pre-existing gap, and this PR's EMPTY-namespace Sid keeps the datatype IRI recoverable for it where thexsd:stringfallback destroyed it. Follow-up: Multi-ledger datasets: datatype-constraint Sid is never re-encoded per graph, so cross-ledger custom-datatype matches silently drop #1771.Gates
cargo test -p fluree-db-query(1,485 lib + integration bins) and-p fluree-db-sparql(641): green.-p fluree-db-api --features native:it_literal_identity,it_fastpath_1652_regression,it_membership_join_list_rows,it_cyclic_bgp_string_dict,it_query_sparql_indexed: green.testsuite-sparql, both-way registers): 36/36 groups green.cargo clippy --all-targets --no-deps -- -D warningson all three touched crates;cargo fmt --all --checkclean.