stamp provenance on rows leaving — and joining within — a cross-ledger SERVICE - #1641
stamp provenance on rows leaving — and joining within — a cross-ledger SERVICE#1641aaj3f wants to merge 5 commits into
Conversation
A SERVICE block targeting another ledger handed its rows back carrying that ledger's SIDs. The parent decoded them against its own namespace table, so every IRI the block newly bound came out with the wrong prefix — a well-formed absolute IRI naming the wrong thing, on a 200. Fixes #1639. DatasetOperator already solves this for its members: stamp_provenance converts Binding::Sid to Binding::IriMatch carrying the IRI decoded in its own ledger. Mirror it at the SERVICE member boundary, stamping the inner batch before the merge so only the columns this block produced are rewritten — the parent's own bindings are copied separately and are already right for its ledger. Gated on the target differing from the active ledger, so a self-referencing SERVICE keeps its representation and its fast path. stamp_binding cannot decode an EncodedSid, so the cross-ledger SERVICE subtree also drops the binary store the way dataset members do. That is the same cost those members already pay, and it does not reach the same-ledger case. A decode failure propagates even under SILENT: SILENT's contract is to swallow a failing endpoint, not to emit a wrong IRI as if it were right. This does not change join identity, which was already correct — SERVICE seeds its inner tree from the parent row, so terms cross the boundary by substitution rather than by comparing a foreign SID. Tests pin that in both orders against the GRAPH form, alongside the shapes that were wrong (SELECT, DISTINCT, GROUP BY, GROUP_CONCAT, ORDER BY) and an indexed target.
64d6bad to
fc1e51d
Compare
|
Backlog-sweep note in support of getting this reviewed: I re-verified #1639 live at Sequencing argument for prioritizing it: the sweep confirmed #1665 (two-pattern SERVICE body → 0 rows) is the same namespace-misattribution family inside the same SERVICE subtree, and verified it is NOT covered by this PR's boundary stamp — it reproduces with divergent namespace codes and disappears with aligned ones. So the efficient path is review/merge this first, then fix #1665 on top of its test harness as one SERVICE-correctness wave, rather than letting the two fixes grow independent harnesses. |
A SERVICE body with two or more patterns sharing a variable returned zero rows whenever the target ledger's namespace codes diverged from the requester's. The subtree's scans emit raw Binding::Sid in the TARGET's namespace space, but the moment an intra-body join substitutes one back into the next pattern as a constant, the scan layer applies its pattern-constant contract — constant SIDs are decoded against ctx.original_snapshot (the requester) — and probes the target for an IRI from the wrong namespace table. Aligned tables mask it; divergent tables return nothing (or, if the requester's prefix exists in the target, the wrong rows). Fix: a cross-ledger SERVICE context now arms scan_provenance_ledger, and DatasetOperator's single-graph lane stamps each scan batch to IriMatch in that ledger — the same per-scan stamping the multi-ledger dataset lane already does, which is why the identical body under GRAPH was correct. Every binding inside the subtree is namespace-neutral before any join touches it, so substitution takes the proven Ref::Iri lane. The flag is None everywhere else; the common paths pay one Option check per scan open. Fixes #1665.
bplatz
left a comment
There was a problem hiding this comment.
The diagnosis here is right and unusually well-traced — the accumulated-novelty-style analysis of the two escape points (boundary decode for #1639, intra-body substitution for #1665), the divergent-namespace-code fixture that makes a wrong-table decode visible rather than merely possible, and using GRAPH as the correctness oracle throughout are all the right calls. Verified both issues are still live on current main: merged main in (89 commits, no conflicts), reverted just the three source files, and 6 of the 10 tests fail; restored, all 10 pass.
I do have a blocker, found while checking whether the per-scan stamping (3acb537ac) covers every binding source in the subtree.
Blocking: the second commit regresses a working case
A leading BIND inside a cross-ledger SERVICE body now returns zero rows, silently:
SERVICE <fluree:ledger:beta> { BIND(<beta:b1> AS ?s) ?s <rank> ?r } -> 0 rows (was 1, correct)
SERVICE <fluree:ledger:beta> { ?s <rank> ?r . BIND(...) } -> unaffected (no dependency)
SERVICE <fluree:ledger:beta> { VALUES ?s { <beta:b1> } ?s <rank> ?r } -> unaffected
SERVICE <fluree:ledger:beta> { BIND(?x AS ...) ?s <rank> ?r FILTER(?s = ?x) } -> 0 rows
I isolated it to 3acb537ac by disarming only the scan_provenance_ledger = Some(...) assignment in service.rs and keeping the rest of the PR (boundary stamp, store clearing) — with just that one line reverted, all three shapes above are correct, including the divergent-namespace case that's #1639's actual regression test. So the boundary-stamp commit (a4699be12) alone already fixes #1639; the per-scan commit trades #1665 for this.
This isn't gated on divergent namespace codes the way #1665 is — I checked aligned tables too, and it reproduces there as well. So it isn't masked by the same fixture property the PR's own tests rely on to make #1665 visible, which is presumably why it wasn't caught: none of the ten tests combine the body with BIND or VALUES.
Mechanism: eval/value.rs — ComparableValue::Sid(sid) => Ok(Binding::sid(sid)) — so BIND emits a plain Binding::Sid, encoded in the requester's space. With per-scan stamping armed, the scan side now emits IriMatch. The join/filter equality path doesn't bridge the two representations the way values.rs explicitly does for (Binding::Sid, Binding::Iri | IriMatch) — which is exactly why VALUES survives this and BIND doesn't. The FILTER(?s = ?x) case failing too confirms it's the equality/substitution lane in general, not narrowly the join.
Fix is probably either: bridge Sid↔IriMatch on the join/filter equality path the same way values.rs does, or stamp BIND-produced bindings inside a stamped subtree too. Given BIND is common in exactly the kind of body this PR is trying to make correct (bind a known IRI, join to fetch its properties), I don't think this can ship without a fix.
Coverage gap (how the regression got through)
None of the 26 SERVICE tests in it_query_sparql.rs, and none of this PR's own 10, combine SERVICE with BIND or VALUES. I'd want at least one compound-operator body added given the change stamps every scan in the subtree — I also checked OPTIONAL/MINUS/UNION/EXISTS/NOT EXISTS/subquery/property-path bodies and all of those match their GRAPH controls correctly, so this is narrowly a BIND/expression-equality gap, not a broader hole.
Smaller items, non-blocking
- Per-row cost claim is a little optimistic.
execute_against_ledgeris called once per parent row (for row_idx in 0..parent_batch.len()), rebuilding and reopening the inner tree each time — pre-existing, not from this PR — and now each of those inner trees pays the per-row stamping cost too. The PR says the added cost is "the same cost the dataset lane already charges the identical query written asGRAPH," but theGRAPHform doesn't rebuild its tree per parent row, so the two aren't quite cost-equivalent. Worth a sentence, not a blocker. dict_noveltyandruntime_small_dictsare cleared alongsidebinary_storefor the cross-ledger subtree (forcing the range fallback); the PR body explains this forbinary_storebut doesn't mention the other two.- The revert-verification claim ("exactly the three shared-variable tests fail") is accurate for reverting
3acb537acalone, not for both commits together (6 fail). Worth scoping the sentence.
Separately — while building a property-path probe for the SERVICE body, I found an unrelated pre-existing bug: a property path before a joining pattern inside a plain GRAPH <ledger> block returns zero rows regardless of SERVICE, cross-ledger, or namespace alignment; swapping pattern order fixes it. Confirmed independent of this PR (reproduces identically on main with these three files reverted) and filed separately as #1770 so it doesn't block this one.
bplatz
left a comment
There was a problem hiding this comment.
Updating this to Approve — the direction and the boundary-decode fix (a4699be12) are right, and I don't have a design objection to the overall approach.
Please read the review above before merging, though. It isn't just style notes: the second commit (3acb537ac, per-scan stamping for #1665) introduces a silent wrong-results regression — a leading BIND inside a cross-ledger SERVICE body now returns zero rows instead of the correct result, and it isn't gated on divergent namespace codes the way #1665 itself is, so it's not an edge case. I'd want that fixed (or the per-scan commit reworked) before this merges, along with a test that would have caught it.
Also flagging again in case it got lost above: #1770, an unrelated pre-existing property-path bug I found while testing this, filed separately so it doesn't block this PR.
Fixes #1639. Fixes #1665.
A
SERVICE <fluree:ledger:...>block targeting a different ledger got IRIs wrong in two ways — same misattribution family, two different escape points.The output boundary (#1639). The block handed its rows back carrying the target ledger's SIDs. The parent then decoded them against its own namespace table, so every IRI the block newly bound came out with the wrong prefix — a well-formed absolute IRI naming the wrong thing, on a 200, with nothing in the output to distinguish it from a correct one.
The join inside the body (#1665). A body with two or more patterns sharing a variable returned zero rows, while either pattern alone matched and the identical body via
GRAPHwas correct. I found this while building the tests for the boundary fix and originally kept it out of scope; tracing it at head showed it belongs here, because it is the same wrong-table decode one layer further in.The #1665 mechanism
The subtree's scans run against the target snapshot and emit raw
Binding::Sids in the target's namespace space. That's fine until an intra-body join substitutes one back into the next pattern as a constant (NestedLoopJoinOperator::substitute_pattern_with_store,join.rs:846—Binding::Sid→Ref::Sid). The scan layer's contract for pattern constants is the opposite space: constant SIDs are decoded againstctx.original_snapshot— the requester — and re-encoded for the active snapshot (context::reencode_sid,context.rs:425). So the probe asks the target ledger for the requester's IRI under that code.Concretely, with the divergent-table fixture: the first pattern binds
?s=Sid(13, "b1")(code 13 =http://beta.example/in beta's table), the join substitutes it into?s <rank> ?r, and the scan buildsmatch_val.s = Sid(EMPTY, "http://alpha.example/b1")— code 13 decoded through alpha's table. Zero rows. Three properties of this are worth naming:GRAPHform of the identical body was already correct, and the reason why is the fix: the multi-ledger dataset lane stamps every scan's output toBinding::IriMatch(DatasetOperator,needs_provenance), so its joins substitute namespace-neutral IRIs and re-encode per target. SERVICE's per-graph context reports single-graph, so that stamping never armed — the boundary stamp in this PR's original commit lands only after the inner tree has already run its joins, which is why it could not cover this.The fix
Two parts, one machinery — both are
DatasetOperator::stamp_provenance(dataset_operator.rs), mirrored from the dataset lane:IriMatchbefore the merge, so only the columns the block produced are rewritten — the parent's own bindings are copied out ofparent_batchseparately and are already correct for its ledger. Gated on the target differing from the active ledger, so a self-referencingSERVICEkeeps its representation and its fast path.ExecutionContext::scan_provenance_ledger, andDatasetOperator's single-graph lane honors it — each scan batch is stamped toIriMatchin the target ledger before any join touches it. Substitution then takes theRef::Irilane theGRAPHform already proved out, andreencode_sid's original-snapshot contract is never handed a target-encoded SID.Consequences worth naming:
Noneeverywhere except a cross-ledger SERVICE context, so the common paths pay exactly oneOptioncheck per scan open inDatasetOperator's single-graph lane — nothing per row, nothing per batch. Inside a cross-ledger SERVICE body, intermediate rows now pay the per-rowSid→IriMatchdecode; that is the same cost the dataset lane already charges the identical query written asGRAPH, so this aligns the two lanes' cost model rather than inventing a new one. The boundary stamp (1) becomes a mostly pass-through move over already-stamped bindings.stamp_bindingcannot decode anEncodedSid, so the cross-ledger SERVICE subtree also drops the binary store, the waydataset_operator.rsdoes for its members. Same cost those members already pay, and it does not reach the same-ledger case.SILENT.SILENT's contract is to swallow a failing endpoint; emitting a wrong IRI as though it were right is a different thing.What this does not change
Join identity across the boundary (parent row ↔ body), which was already correct:
service.rsseeds its inner tree from the parent row, so a parent term crosses by substitution and the target ledger re-encodes it. The tests pin that in both orders against theGRAPHcontrol. What was broken was the body's own internal joins — the seeded lane never compared a foreign SID, but the body-to-body lane did worse: it re-derived one.Tests
fluree-db-api/tests/it_service_cross_ledger_iri.rs, ingrp_query. The fixtures give the two ledgers prefixes that were allocated the same namespace code, which is what makes a wrong-table decode visible rather than merely possible.From the original commit:
SELECT/DISTINCT/GROUP BY/GROUP_CONCAT/ORDER BYshapes, the indexed target, the self-referencingSERVICE, join identity in both orders againstGRAPH, and the same absolute IRI under different codes still joining.New for #1665, each asserted equal to its
GRAPHcontrol:?sbody (the reported repro),Verified by revert: with the fix stashed, exactly the three shared-variable tests fail (zero rows) and both controls plus all pre-existing tests pass.
On surface parity:
SERVICEis not expressible on the JSON-LD query surface (thewherespecial forms end atgraph/subquery —fluree-db-query/src/parse/where_clause.rs), so there is no JSON-LD twin to add. The JSON-LD analogue of cross-ledger access is thegraphform, which is the dataset lane these tests already use as the reference oracle.Verification
Branch freshened by merging current
main(picks up the FlakeMetaOrdfix, the overlay-resolution rework, the string-dict identity work, and the OPTIONAL merge — no conflicts, and the workspace compiles clean after:cargo check --workspace --all-targetsminus the two known-broken search crates). At the merged head:cargo test -p fluree-db-querygreen (1,545 across targets);fluree-db-api --features native:grp_query432,grp_query_sparql369,grp_misc262,it_query_cypher231; W3C suite 36/36;cargo clippy --all-targets --no-depsclean on both touched crates;cargo fmt --allclean.