fix: make IN/NOT IN match encoded resources, price the row lanes max_fuel could not see - #1681
Conversation
FILTER(?v IN (<iri> ...)) matched nothing when ?v carried an index-encoded binding: eval_in compared the row's EncodedSid/Sid against the constant Iri through rdf_term_equal, whose Resource arm compares representations, not resources — so every element evaluated to "not equal" and the filter silently dropped all rows. NOT IN dually kept everything. The =/!= operators were immune because CompareOp::eval consults the IRI-binding fast path (fast_eq_ne_for_iri_bindings) before falling back. Restructure that fast path into a directional core returning a three-way outcome (definitive equality, test-side unbound, element-side unbound) so a shared membership evaluator for IN and NOT IN can probe each element through the same encoded/Sid/IRI representations — with the same per-query const-sid memoization — before materializing the test value. Elements the fast path cannot decide, including demotable element-eval errors that must stay pending, fall back to the generic comparable path with unchanged semantics. Found while reproducing BUG-values-join-planner: the reported FILTER ... IN workaround returned 0 rows on this lineage. Regression tests cover one- and multi-element IN over refs, subject-position IN, an absent IRI, and NOT IN exclusion.
BUG-values-join-planner: binding both endpoints of an edge star with two VALUES clauses ran 1,600x slower than the equivalent FILTER ... IN (9.3s vs 3.3ms on a 129k-edge corpus), returning identical rows. The star block's planner defers every non-subject VALUES to a post-join ValuesOperator (to preserve property-join fusion), so a two-row VALUES constrained nothing: the star drained the driving predicate's whole extent, materialized every row — wide snapshot payloads included — and the VALUES filtered afterwards. The hand-written membership filter, strictly less expressive, inlined into the fused star and pruned rows before materialization. Close the asymmetry by construction: when a fresh star block's VALUES all lower — single variable, bound by a star triple, rows distinct fully-bound resources — rewrite each into FILTER(?v IN (...)) at block assembly, cells lowered exactly as SPARQL lowers the hand-written filter. The block then takes the same filter-aware build path, making the two forms the same plan. Declines preserve exact join semantics: UNDEF cells (match-any), duplicate rows (multiplicity), multi-var VALUES (column correlation), literal cells, subject-binding VALUES (seeding beats filtering), and mixed blocks (all-or-nothing, so an unconvertible sibling keeps today's deferral). Repro dropped from 589ms to 3.8ms at N=2k in-test; plan-shape pins live in where_plan unit tests, join-semantics pins in it_values_object_bounds.
A query that drained and materialized a whole predicate extent — the BUG-values-join-planner repro burned 589ms of scan + join + materialization — reported floor-level fuel (~1.0): index leaflet touches are thousands of rows coarse, late materialization skips dict touches, and no per-row work in the scan/join lanes crossed a charging surface. CPU-bound queries were therefore invisible to max_fuel limits and to fuel-based accounting. Charge PER_ROW_MICRO_FUEL (0.001 fuel) per row at three origins, always once per batch/chunk at the existing cancellation boundaries — never per iteration inside fused merge loops (hot-loop purity: a never-taken in-loop branch measured +5-15% end-to-end): - BinaryScanOperator: cursor-emitted rows, charged per batch. This is the choke point every scan-fed lane shares (NLJ chains, property-join inner scans, filter scans). Rows the encoded prefilters drop inside the cursor are never emitted and stay uncharged — their cost is nanoseconds and the leaflet touch already prices the I/O. - PropertyJoinOperator: batched subject-probe and SPOT star-walk matches, which bypass the scan operator entirely. - ValuesOperator: join input rows, pricing the input x value-rows work that origin charges cannot see (the W4-3 VALUES-over-graph-source shape). An unfiltered 1,500-row star now reports 2.51 fuel (floor + rows + leaflets) instead of 1.01; a VALUES join over the same stream stacks its input charge on top. Both charges are pinned by drained_rows_are_visible_to_fuel, each proven to fail with its charge disabled. The docs/query/tracking-and-fuel.md cost ladder gains the three rows.
aaj3f
left a comment
There was a problem hiding this comment.
The IN/NOT IN fix is great, @bplatz — a directional core with a three-way outcome is the right shape for letting IN know which side was unbound, it reuses the const_sid_cache memo rather than growing a parallel one, and it turns a silent 0-rows into the right answer 2.8× faster on the encoded path (I checked it fixes the JSON-LD in filter too, since it's the same eval_in).
The fuel charges are placed exactly where they should be — once per batch at boundaries that already poll cancellation, units == 0 short-circuited — and since max_fuel is opt-in everywhere I looked (Fluree AI's fluree-max-fuel header, the server's header extractor), the behavior change lands only on callers who asked for a limit and now get an honest number.
I am struggling a bit reconciling the headline claim: on this base the VALUES lowering doesn't seem to do what the body and docs say. The repro shape never reaches it (the singleton fold from a8e7d79 handles VALUES ?a { <A> } first and the all-or-nothing gate then declines the block — the HEAD plan is ValuesOperator → ValuesOperator → PropertyJoinOperator, same as BASE), and with convert_star_values_to_membership_filters short-circuited all six integration tests still pass.
Where it does fire, an IN filter carries no object_bounds, so analyze_property_join_plan drops the block off the fused star onto the NLJ chain: ~1.3× over the deferred path at ~5× the row emissions (201 vs 41 fuel at 20k), while the seeded-VALUES plan the same builder produces when the block happens to carry another filter runs 30× faster at 1.17 fuel. Alongside that, eval_membership now evaluates every element twice and allocates a Vec per row for literal-bound test vars (+20% on a 20k-row string-IN in the A/B against BASE eval code), and two of the four fuel charge sites (property_join.rs:949, :1142) aren't pinned by the test that the body says pins each of them.
Adherence to repo commitments:
- Patterns/abstractions:
⚠️ theINfix extends the existing IRI-binding fast path correctly; the VALUES lowering reinvents the constraint as a filter where the file already has two seed mechanisms (split_values_seeding_star,inline_singleton_values_objects) that keep the star anchored — and the seed path measures 30× faster. - Performance (speed first, memory second): ✖ CRITICAL — per-row
Vecallocation + double element evaluation on literalINlists (logical.rs:123-136/compare.rs:191); converted VALUES blocks leave the fusedPropertyJoinOperatorfor the NLJ chain; no bench covers either shape (query_hot_bsbmhas noINand no multi-row VALUES). - Testing: ✖ the conversion is unexercised by every integration test (all pass with it disabled) and pinned only by a unit test using
Binding::Iricells SPARQL never produces; property-join charge sites unpinned; no JSON-LD twin for a shared-IR fix. Stacked PR —ci.ymland the W3C SPARQL suite have not run on this code. - Conventions: ✔ multi-line, mechanism-first commit bodies; fmt/clippy clean on the touched crates; docs and cost ladder updated — though
docs/design/performance.md:253and the memory fact describe a fused-star plan that isn't built.
Verified locally at branch HEAD (3d8bff3, base 3f1368f): cargo fmt --all -- --check clean; cargo clippy -p fluree-db-query --all-targets -- -D warnings clean; cargo nextest run -p fluree-db-query 1474/1474; it_values_object_bounds 6/6; mutation checks — conversion disabled: unit pin red, all integration tests green; IN fast path disabled: 3 tests red (as claimed); fuel charges: scan and VALUES sites red, both property-join sites green; BASE eval code A/B on a 20k-row ledger for the literal-IN cost.
I'll go ahead and approve so you're unblocked with whatever you decide but...
The fix path I'd suggest: keep commit 1 and commit 3 as they are (with the literal-path early-out and either a probe-lane fuel pin or a trimmed claim), and rework commit 2 to seed bounded multi-row object VALUES rather than lower them to a filter — with an integration shape that converts under SPARQL lowering and an explain assertion on the plan. If the filter lowering stays for a reason I'm not seeing, I'd want the two-multi-row measurement and the fused-star narrative reconciled in the body before this merges — and the W3C suite run against main once #1680 lands.
Pre-existing (not introduced here, named for the record):
- A
VALUESplaced after anOPTIONALthat binds its variable cross-products instead of joining:?ev :e1 ?a ; :snap ?s . OPTIONAL { ?ev :e2 ?b } VALUES ?b { <B> <C> }returns 3000 rows on a 1500-edge ledger where SPARQL says 2 — identical with this PR's conversion disabled (the block has an upstream seed, so the conversion is declined by construction). Silent-wrong-results; it deserves its own issue, and I'd rather it be scoped deliberately than squeezed in here — that's a decision above this PR. ?x NOT IN (…)with unbound?xreturns the row (logical.rs:227-230, vacuously true); the spec has the=error propagate through OR and the FILTER drop the row. The BASE code did the same. Noting only because the three-wayTestUnboundoutcome is the natural place to fix it if we ever want to.
| // the hot-path fork: a conversion adds block filters, which | ||
| // routes the block down the filter-aware path. | ||
| let mut block = block; | ||
| convert_star_values_to_membership_filters(&mut block, operator.is_some()); |
There was a problem hiding this comment.
blocking (CRITICAL, performance/design). The VALUES→FILTER IN lowering moves a converted block off the fused star and onto the sequential NLJ chain, and nothing in the suite exercises it on real input.
An IN filter produces no object_bounds (extract_bounds_from_filters only lifts range/equality comparisons), so analyze_property_join_plan at where_plan.rs:981-982 sees has_selective_anchor == false and sets can_property_join = false. The converted block is therefore built by build_sequential_join_block, not build_property_join_block — explain_sparql on a seeded 20k-edge ledger shows NestedLoopJoinOperator → NestedLoopJoinOperator → DatasetOperator with no PropertyJoinOperator for VALUES ?a {A A2} VALUES ?b {B C} + star. The commit message, docs/design/performance.md:253, and the memory fact all say the filter "inlines into the fused star and prunes rows before materialization"; that plan is not what gets built.
What it costs: at 20k edges (debug build, ratios only) the converted shape runs in 63–94 ms and reports 201 fuel (~10 row emissions per edge); with the conversion disabled the deferred hot path runs in 120 ms at 41 fuel. So the lowering buys ~1.3×, not the 1,600× in the body, and emits ~5× the rows. Meanwhile the very same two VALUES plus an unrelated FILTER(?snap != "zzz") — a shape this function declines — runs in 2.0 ms at 1.17 fuel, because the filter-aware sequential builder seeds the VALUES at the base and drives bound-object probes (ValuesOperator×2 → HashJoinOperator{driving-est 4, cost-wins} → NLJ → NLJ). The winning plan already exists in this builder; the lowering steers away from it and toward the hand-written-filter plan, which is the slow one. The "same plan by construction" claim also depends on clause order: VALUES ?b … written before VALUES ?a … gives 1.01 fuel / 40 ms, the other order 201 fuel / 63 ms, because reorder_patterns runs on the VALUES-bearing list before the conversion rewrites it.
And the tests don't see any of this. With convert_star_values_to_membership_filters short-circuited (if true { return; } as its first statement), all six tests in it_values_object_bounds.rs still pass. The PR's own repro shape (VALUES ?a { <A> }, one row) never reaches the conversion on this base: inline_singleton_values_objects at :2139 folds the one-row VALUES into the triple object first, ?a is no longer star-produced, and the all-or-nothing gate declines the block — the HEAD plan for that shape is ValuesOperator → ValuesOperator → PropertyJoinOperator, identical to BASE, and it is fast (0.7–2.9 ms, 1.04 fuel) because of the fold. The only test that goes red under the mutation is the unit plan-shape test, and it uses Binding::iri(…) cells, which the SPARQL lowering never emits (fluree-db-sparql/src/lower/term.rs:467-474 always produces Binding::Sid, and errors on an unknown namespace).
Fix direction: lower a bounded multi-row object VALUES to a seed — the mechanism split_values_seeding_star already uses for subject VALUES and the singleton fold uses for one-row object VALUES — rather than to a filter, so the driving triple gets a bound-object set and the star keeps its anchor; the 2.0 ms / 1.17 fuel plan above is the target, and the existing seeded-VALUES path reaches it today. Whichever way it goes, the integration test needs a shape that actually converts under SPARQL lowering (two multi-row VALUES with Sid cells), an explain-level assertion on the plan, and the docs/commit narrative corrected to the plan that is built. Happy to talk through the seed-vs-filter choice, but the coverage gap and the narrative need to close before this merges.
There was a problem hiding this comment.
Reverted in 412aa7db8. I reproduced your finding before acting: instrumenting the conversion shows 0 firings across all six tests that shipped with it, and 1 firing across the whole 349-test grp_query_sparql group — in multi_row_values_object_is_a_set_not_a_constant, which predates the PR. Mechanism confirmed as you describe: inline_singleton_values_objects retains the singleton VALUES after folding it into the triple object, so its var leaves star_vars, membership_filter_from_values declines it, and all-or-nothing declines the block. Also confirmed the object_bounds half — extract_range_constraints only handles Eq/Lt/Le/Gt/Ge/And, so Function::In yields nothing and has_selective_anchor is false.
Your point about the seeded plan already being the fast one (2.0ms / 1.17 fuel) is what convinced me to revert rather than patch: the winning plan exists in that builder and the lowering steered away from it. Re-landing it as a seed is a separate change. The memory fact now records both failure modes so it does not get re-tried in the same shape.
| ) -> Result<Membership> { | ||
| use super::compare::{fast_in_membership_for_iri_bindings, FastEqOutcome}; | ||
|
|
||
| let mut unresolved: Vec<&Expression> = Vec::new(); |
There was a problem hiding this comment.
blocking (CRITICAL, hot path). eval_membership allocates a Vec per row and evaluates every list element twice whenever the test binding is a literal — the common FILTER(?status IN ("a","b")) shape.
fast_eq_iri_binding_directional evaluates other_expr at compare.rs:191 before matching on the binding variant at :196-306, so for a Lit/EncodedLit test var every element is materialised (Const(String) → Arc::from allocation), discarded via the _ => Ok(None) arm, pushed into unresolved (logical.rs:136, heap-allocating the Vec on the first push), and then evaluated again in the generic loop at :150-165. The old eval_in did one evaluation per element and no Vec.
Measured (BASE eval/logical.rs + eval/compare.rs restored into the HEAD tree, 20k rows, debug build, two runs each): FILTER(?snap IN ("nope1","nope2","nope3")) 60.0/59.9 ms → 72.2/73.4/73.3 ms at HEAD (+20%); FILTER(?n IN (1,2,3)) 34.7/35.2 → 38.9–43.2 ms. The resource path is the win the PR is after (107 ms and wrong → 38 ms and right), so this is a fixable side-effect, not a reason to lose the fix.
Fix: decide on the binding variant before touching any element. In fast_eq_iri_binding_directional, hoist the variant check above the eval_to_comparable at :191 (return Ok(None) immediately unless the binding is EncodedSid | Sid | Iri | IriMatch | EncodedPid); and in eval_membership, skip the fast loop entirely — no Vec — when args[0] isn't a var with a resource-flavoured binding, falling straight into the generic loop over &args[1..]. That restores the BASE cost for literal lists and keeps the encoded path.
There was a problem hiding this comment.
Fixed in 4af3adbd1, exactly the two-part shape you prescribed: the flavor check is hoisted above the element evaluation, and eval_membership skips the fast loop entirely (no Vec) when args[0] is not a var with a resource-flavored binding, falling straight into the generic loop over &args[1..].
The generic loop moved into generic_membership, shared by both entries into it — the whole element list when the fast path cannot apply, the undecided remainder when it can — so the two cannot drift in their error semantics. An unbound test var answers false to the flavor question and is caught by the generic path's own unbound check, which reports the same TestUnbound the probe did.
binding_is_resource_flavored is unit-pinned against the match arms it mirrors, since a new resource-flavored Binding added to one list and not the other silently costs either the fast path or the early-out.
| probe_ops.as_mut(), | ||
| )?; | ||
| scan_rows_total += probe_matches.len() as u64; | ||
| charge_scan_rows(ctx, probe_matches.len())?; |
There was a problem hiding this comment.
This applies to both charge_scan_rows call sites — here and at fluree-db-query/src/property_join.rs:1142.
blocking (testing). Neither property-join charge site is pinned; the body's "each fuel charge disabled fails its assertion" holds for two of the four sites.
drained_rows_are_visible_to_fuel goes red with the binary_scan.rs:2499 charge removed (star reports 1.01) and with the values.rs:276 charge removed (2.51 vs 2.51), but stays green with both charge_scan_rows calls removed — the 1,500-row star in the test is served by BinaryScanOperator emission alone, so the batched-probe and SPOT-walk lanes contribute nothing to the assertion. Either add a star shape that takes the batched-probe / SPOT-walk lane (a bound-object anchor so can_property_join is true, with enough subjects to cross the batching threshold) and assert fuel rises with the probe count, or drop the claim from the body and the commit — but the two uncovered sites are exactly the lanes the commit singles out as bypassing the scan operator.
There was a problem hiding this comment.
You were right, and chasing it turned up why those sites were awkward to pin: the charges sat at call sites rather than at the thing being paid for, and NestedLoopJoinOperator — which reaches the index through the same helpers — never got one at all. Its whole probe side was free.
So rather than pin these two, 904e835ff moves the charge into batched_subject_probe_binary and batched_subject_star_spot, and removes both charge_scan_rows calls here as now-duplicate. Every caller pays alike, and the next operator to reach the index cannot be free by omission. scan_matches and the object-driven flush keep boundary charges of their own because they read leaflets directly rather than through those helpers.
nested_loop_join_probe_rows_are_visible_to_fuel pins the lane by proportionality and goes red under mutation. Measured repricing: a 400-subject seeded star 1.001 -> 2.201, a plain three-triple star 2.51 -> 5.51 — exactly the probe rows those plans always expanded. Granularity is unchanged: one fetch_add per batch off a count the loops already tally for their debug lines.
| VALUES ?b {{ <{b}> UNDEF }}\n\ | ||
| ?ev ns:entity1 ?a ; ns:entity2 ?b ; ns:snap ?snap }}" | ||
| )) | ||
| .await; |
There was a problem hiding this comment.
optional. The IN-over-refs bug is JSON-LD-visible too: ["filter", ["in", "?b", [["iri", B], ["iri", C]]]] returns 0 rows with the fast path disabled and 2 at HEAD, through the same eval_in. Per the shared-IR parity rule we should probably add the JSON-LD twin of filter_in_matches_encoded_iri_bindings in it_query.rs — it's a dozen lines, and it would document that this was never SPARQL-specific. Minor and non-blocking, but if you agree it's right I'd rather see it folded in now than lost in the backlog.
There was a problem hiding this comment.
Agreed and folded in — 406af452b adds jsonld_filter_in_matches_encoded_iri_bindings, the twin of the SPARQL test, and it fails the same way under the same mutation (fast path disabled -> 0 rows). It lives beside the SPARQL one so it shares the fixture rather than re-seeding in it_query.rs.
One thing worth recording from writing it: the s-expression form needs (in ?b [(iri "...")]). A bare <iri> inside the list literal does not lower to an IRI constant — (= ?b <iri>) returns 0 rows for the same reason — so that syntax fails quietly rather than erroring. Cost me a diagnostic round to find, hence the note in the commit body.
| /// `None` when the VALUES is not expressible as a filter (multi-var, var not | ||
| /// bound by the star, UNDEF or literal cells, duplicate rows). | ||
| /// | ||
| /// Cells lower exactly as SPARQL lowers the hand-written filter: a known- |
There was a problem hiding this comment.
nit (doc accuracy). "Cells lower exactly as SPARQL lowers the hand-written filter: a known-namespace Sid cell becomes a Ref constant" — SPARQL never emits a Const(Ref) in an expression; lower/expression.rs:43-55 wraps every IRI as IRI("<full iri>") and the encoding happens at eval in eval_iri. The Const(Ref) the conversion emits is fine (it's the Cypher lowering's shape, and cheaper per row than the IRI() call), it just isn't "exactly as SPARQL". Worth rewording so the next reader doesn't go looking for a lowering path that doesn't exist.
There was a problem hiding this comment.
The comment and the code it described are gone with the revert — but you were right on the substance, and it is worth having on the record: SPARQL never emits a Const(Ref) in an expression, lower/expression.rs wraps every IRI as IRI("<full iri>") and the encoding happens at eval. Any future seed-based version of this needs to not repeat the "exactly as SPARQL lowers" claim.
| return None; | ||
| } | ||
|
|
||
| // Duplicate detection keys on the resource identity, not the Binding |
There was a problem hiding this comment.
nit. "the same IRI carried once as Sid and once as Iri still counts as a duplicate" — the keys are (namespace_code, local name) for Sid and (u16::MAX, full iri) for Iri, so they can't collide. Probably unreachable from any front-end (one query lowers one IRI one way), but the comment claims a property the code doesn't have; either key both on the expanded IRI or trim the comment.
There was a problem hiding this comment.
Also gone with the revert. For the record you were right that the keys could not collide — (namespace_code, local) for Sid versus (u16::MAX, full iri) for Iri — so the comment claimed a property the code did not have. Worth remembering if the seed version needs dedup: key on the expanded IRI.
| /// bound resource (Sid / IRI). UNDEF cells join as match-any and duplicate | ||
| /// rows multiply solutions — a filter can express neither, so both decline | ||
| /// and keep the `ValuesOperator` join. | ||
| fn convert_star_values_to_membership_filters(block: &mut InnerJoinBlock, has_upstream_seed: bool) { |
There was a problem hiding this comment.
question, not a suggestion. The gate declines any block with an existing FILTER/BIND, and per the measurements above that's currently the fast branch (the VALUES get seeded). If the lowering stays, it seems maybe we'd want the decline to be the default rather than the exception — i.e. the conversion is only ever worth taking when it beats the seed, and I couldn't find a shape where it does. This is really the same conversation as the first blocking item.
There was a problem hiding this comment.
Answered by reverting. I could not find a shape where the conversion beats the seed either, and your measurements are what made that concrete — the decline was not the exception, it was the only case that was ever right. Re-landing this as a seed that keeps the star anchored is the direction, in its own PR.
This reverts commit 85f5236047d0d5cba4bee73d4a52a4dcbdc5d5f4.
The conversion does not fire on the shape it was written for, and where
it does fire it is slower than the plan it replaces.
`inline_singleton_values_objects` rewrites a singleton VALUES into the
triple object but retains the VALUES pattern, so its variable is no
longer produced by any star triple. `membership_filter_from_values`
declines it, and the all-or-nothing gate then declines the whole block.
The repro — `VALUES ?a { <A> }` alongside `VALUES ?b { <B> <C> }` — is
exactly that shape, so its plan is unchanged from BASE. Instrumenting
the conversion confirms it: zero firings across all six tests that
shipped with it, and one firing across the whole 349-test SPARQL group,
in a test that predates it.
Where it does fire, `Function::In` yields no range constraint from
`extract_range_constraints`, so the block carries no `object_bounds`,
`has_selective_anchor` is false, and it leaves the fused
`PropertyJoinOperator` for the NLJ chain — measured at ~1.3x the time
and ~5x the row emissions of the deferred path it replaced.
The constraint is worth pushing into the scan, but as a seed that keeps
the star anchored rather than a filter that unanchors it. Re-landing
that needs an integration shape that converts under SPARQL lowering and
an `explain` assertion on the plan, neither of which this had.
The IN/NOT IN correctness fix and the fuel accounting are independent
and stay.
The resource fast path can only reach a verdict when the test side is a variable bound to a resource-flavored binding — every other flavor falls out of `fast_eq_iri_binding_directional`'s trailing `_ => Ok(None)`, but only after it has already evaluated the element operand. On the `IN` path that meant a literal-bound `?v` evaluated every element twice, once in the probe and once on the generic path, and collected all of them into a per-row `Vec` on the way there. Ask the flavor question once, before any element is touched, and route straight to the generic loop when the answer is no. The loop moves into `generic_membership`, shared by both entries into it — the whole element list when the fast path cannot apply, the undecided remainder when it can — so the two cannot drift in their error semantics. An unbound test variable answers `false` to the flavor question and is caught by the generic path's own unbound check, which reports the same `TestUnbound` the probe did. `binding_is_resource_flavored` is unit-pinned against the match arms it mirrors: adding a resource-flavored `Binding` to one list and not the other silently costs the fast path or the early-out. Also correct two claims from the fuel commit: - Its property-join charges are exercised end-to-end by a new hub-star test, but they are NOT pinned by it. Their contribution is 0.80 of 6.61 fuel (800 rows), and the IO touch charges over the same leaflets dominate — deleting both charges leaves any assertion on that query green. Isolating them needs two shapes with identical IO and different row counts, which this index layout cannot produce; the test says so rather than implying otherwise. - A third row lane is still uncharged: a star whose SUBJECT is bound by VALUES is seeded, not joined, and crosses none of the charging surfaces. 400 subjects expand to 400 rows for 1.001 fuel — floor level, invisible to max_fuel. Recorded as a known gap in the cost ladder rather than fixed here.
|
You were right on every count, and the VALUES commit is gone — The conversion. I reproduced your finding before acting on it and it's worse than "doesn't fire on the repro". Instrumenting the conversion: 0 firings across all six tests that shipped with it, and 1 firing across the whole 349-test That also means the commit's "589ms → 3.8ms at N=2k" was unattributable — the conversion never ran on that shape. Removed from the body. The double evaluation. Confirmed by reading: the The unpinned fuel sites. Confirmed — deleting both left the test green. I added a hub-star test that drives 800 rows through the SPOT lane, but I want to be straight about what it does: it is coverage, not a pin. The charges contribute 0.80 of 6.61 fuel there and the IO touch charges over the same leaflets swamp them, so deleting both still leaves it green. Isolating them needs two shapes with identical IO and different row counts, which I couldn't construct on this index layout. I verified the charges by hand instead (5.81 without, 6.61 with) and the test's doc comment says exactly that rather than implying a pin — same correction we just made on #1679. One more thing that fell out of building that test. A star whose SUBJECT is bound by VALUES is seeded rather than joined, and its rows cross none of the charging surfaces: 400 subjects → 400 rows → 1.001 fuel. Same bug class the fuel commit exists to fix, still open in that lane. I recorded it as a known gap in the cost ladder rather than fixing it here, since adding a charge is a behavior change and Your pre-existing find is filed as #1690 with a reproduction — I get 1500 rows where the equivalent Left alone deliberately:
|
`NestedLoopJoinOperator` reads leaflets itself — `scan_matches` for the subject-driven lane, its own POST walk for the object-driven flush — and neither crossed a charging surface. Only the leaf scans and the two lanes priced earlier paid anything, so a join's probe side was free no matter how many rows it expanded. Most visible where nothing masked it: a star whose subject is seeded by `VALUES` sits on a `ValuesOperator` over `EmptyOperator`, which charges per *input* row (one), so 400 subjects returned 400 rows and reported 1.001 fuel — the query floor and nothing else. An ordinary `?s :p1 ?a ; :p2 ?b ; :p3 ?c` star was undercounted the same way, just less obviously: its base scan paid for 1500 rows while the two joins above expanded 3000 more for free. The charge goes in the shared primitive, not at the call sites. `batched_subject_probe_binary` and `batched_subject_star_spot` are reached from both `PropertyJoinOperator` and `NestedLoopJoinOperator`; pricing them per call site had billed the same read in one operator and not the other, so a query's cost depended on which lane the planner picked. Charging inside them makes every caller pay alike and means the next operator to reach the index cannot be free by omission — the two call-site charges in `property_join.rs` are removed as now-duplicate. `scan_matches` and the object flush keep boundary charges of their own because they read leaflets directly rather than through those helpers. Granularity is unchanged: one `fetch_add` per batch, using a row count the loops already tally for their debug lines. Nothing is added inside a leaflet merge loop (hot-loop purity), and `consume_fuel` short-circuits before the atomic when tracking is off. This raises *reported* fuel, not query time. Measured on a 1500-edge fixture: a 400-subject seeded star 1.001 -> 2.201, a plain three-triple star 2.51 -> 5.51. Both deltas are exactly the probe rows those plans always expanded. Callers who set `max_fuel` against the undercount will need to re-tune; the work was always happening.
|
Heads up @aaj3f — this PR grew after your approval, so it needs another look. Chasing your "two of the four fuel charge sites aren't pinned" note turned up the reason those sites were hard to pin: the charges had been placed at call sites rather than at the thing being paid for, and one operator simply never got one. It showed up on a subject-seeded star, where nothing masked it: So the fix moved: Granularity is unchanged and I kept the hot-loop rule in front of me: one
Both deltas are exactly the probe rows those plans always expanded. The part worth your judgement: this reprices ordinary queries 2-3x upward, and every
|
`eval_in` is shared IR: JSON-LD `filter` expressions lower to the same `Function::In` and hit the same representation mismatch, so the bug dropped every row there as well. Only the SPARQL form was covered. Twin of `filter_in_matches_encoded_iri_bindings`, and it fails the same way under the same mutation (fast path disabled -> 0 rows). Note for anyone writing one of these: the s-expression form needs `(in ?b [(iri "...")])`. A bare `<iri>` inside the list literal does not lower to an IRI constant — `(= ?b <iri>)` returns 0 rows for the same reason, so the syntax fails quietly rather than erroring.
Problem
BUG-values-join-planner (magna POC):
FILTER(?v IN (<iri> ...))returned 0 rows against index-encoded bindings on current lineage — the workaround the bug report recommends only works on older releases.NOT INdually kept everything.Reproducing it surfaced a second defect: the offending query burned 589ms of scan/join/materialization while reporting 1.01 fuel, so CPU-bound queries in the fused lanes were invisible to
max_fuellimits.Root causes
eval_incompared the row'sEncodedSid/Sidagainst the constantIrithroughrdf_term_equal, whose Resource arm compares representations, not resources — every element silently evaluated "not equal".=/!=were immune becauseCompareOp::evalconsults the IRI-binding fast path first.Fix
fix(query): restructure the IRI-binding fast equality into a directional core with a three-way outcome (definitive equality / test-side unbound / element-side unbound) and routeIN/NOT INthrough it element-by-element, with the same per-query const-sid memoization. Undecidable elements (including demotable eval errors that must stay pending) fall back to the generic path unchanged.feat(query): chargePER_ROW_MICRO_FUELat the row origins that were invisible tomax_fuel. Always once per batch/chunk at existing cancellation boundaries, never per iteration inside fused merge loops.feat(query)(scope extension, see below): charge the join layer's own row lanes, and move the batched-probe charge into the shared primitive so every caller pays alike.fix(query)(review follow-up): the resource fast path only decides for resource-flavored test bindings, but reached that verdict after evaluating the element — so a literal-bound?v IN (...)evaluated every element twice and allocated a per-rowVec. Ask the flavor question once, up front; the generic loop is now shared by both entries into it.What was reverted
The original third commit lowered eligible star-block VALUES to
FILTER(?v IN (...)). It has been reverted — @aaj3f was right that it does not do what it claimed.inline_singleton_values_objectsrewrites a singleton VALUES into the triple object but retains the VALUES pattern, so its variable is no longer produced by any star triple;membership_filter_from_valuesdeclines it and the all-or-nothing gate then declines the whole block. That is exactly the two-VALUES repro shape, so its plan was unchanged from BASE.Function::Inyields no range constraint fromextract_range_constraints, so the block carried noobject_bounds,has_selective_anchorwas false, and it left the fusedPropertyJoinOperatorfor the NLJ chain.The constraint is still worth pushing into the scan, but as a seed that keeps the star anchored rather than a filter that unanchors it. That is a separate PR, and it needs an integration shape that converts under SPARQL lowering plus an
explainassertion on the plan.Verification
IN/NOT IN: mutation-verified — theit_values_object_boundsIN tests fail with the fast path disabled.drained_rows_are_visible_to_fuel; the join-layer charges bynested_loop_join_probe_rows_are_visible_to_fuel(red under mutation, green on restore). The property-join charges are not pinned — a new hub-star test exercises the lane end-to-end (800 rows through the SPOT walk), but their contribution is 0.80 of 6.61 fuel and the IO touch charges over the same leaflets dominate, so deleting both leaves any end-to-end assertion green. Verified by hand instead (5.81 without / 6.61 with); the test's doc says exactly this rather than implying a pin.binding_is_resource_flavoredis unit-pinned against the match arms it mirrors.fluree-db-query+fluree-db-server2011/2011; fullfluree-db-api3690/3691 (sole failure the recurring LocalStack testcontainers flake, which passes in isolation);it_values_object_bounds8/8.Scope extension: the join layer was uncharged
Reviewing the fuel commit turned up something bigger than the three charges it added.
NestedLoopJoinOperatorreads leaflets itself —scan_matchesfor the subject-driven lane, its own POST walk for the object-driven flush — and neither crossed any charging surface. Only leaf scans and the two lanes the original commit touched paid anything, so a join's probe side was free no matter how many rows it expanded.It surfaced on a star whose subject is seeded by
VALUES, where nothing masked it: that plan sits on aValuesOperatoroverEmptyOperator, which charges per input row (one), so 400 subjects returned 400 rows for 1.001 fuel — the query floor and nothing else. But an ordinary?s :p1 ?a ; :p2 ?b ; :p3 ?cstar was undercounted the same way, just less visibly: its base scan paid for 1500 rows while the two joins above expanded 3000 more for free.The charge now lives in the shared primitive, not at the call sites.
batched_subject_probe_binaryandbatched_subject_star_spotare reached from bothPropertyJoinOperatorandNestedLoopJoinOperator; pricing them per call site had billed the same read in one operator and not the other, so a query's cost depended on which lane the planner picked. Charging inside them makes every caller pay alike, and means the next operator to reach the index cannot be free by omission — the two call-site charges inproperty_join.rsare removed as now-duplicate.scan_matchesand the object flush keep boundary charges of their own because they read leaflets directly rather than through those helpers.Granularity is unchanged: one
fetch_addper batch, using a row count the loops already tally for their debug lines. Nothing added inside a leaflet merge loop (hot-loop purity), andconsume_fuelshort-circuits before the atomic when tracking is off. This is a change to reported fuel, not to query time.Both deltas are exactly the probe rows those plans always expanded.
Operator-visible consequence: anyone who tuned a
max_fuellimit against the undercount will need to re-tune, and queries that pass today may start being rejected. The work was always happening — it just wasn't billed.Still uncharged, deliberately:
hash_join.rs,optional.rs,membership_join.rs,graph.rs. They recombine rows already charged at the leaves, so they are an undercount bounded by their inputs rather than free IO.Filed separately
#1690 —
VALUESafter anOPTIONALthat binds its variable cross-products instead of joining (1500 rows where the equivalentFILTERreturns 1). Reported by @aaj3f in review, reproduced and confirmed pre-existing.