Repository navigation
perf(engine): indexed top-k ORDER BY for star BGPs, with #6465 regressions fixed (carries #5983) - #6660
Merged
Conversation
…ted backoff to txn The opt-in txn feature's first-committer-wins OCC has no retry helper today, so every caller writes its own retry loop. Add execute_txn_with_retry (owns the begin/update/commit lifecycle, retries only CommitError::Conflict, never an update error) and decorrelated_jitter_backoff (dependency-free, keeps the feature's zero-new-deps contract). Motivated by the Artifact Keeper datastore migration spike's job-queue coordination pattern (claim one row per txn; jittered backoff decorrelates concurrent retries on a hot row). Updates skills/sparql-query/SKILL.md per the maintenance rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…+LIMIT try_topk_orderby's bounded-heap optimization still fully materialises the inner pattern via eval_modified before selecting the top-k rows -- O(n) in the number of rows currently matching the BGP's equality filters, not in the row budget. That's fine when some pattern is selective, but for a "star" BGP where every pattern matches nearly the same rows (e.g. a job-queue claim: same peer + status, ORDER BY priority LIMIT 1), there is no cheaper seed for the cardinality-based planner to pick, and the cost scales with the whole candidate set on every claim. Profiled via EXPLAIN ANALYZE against a synthetic claim-queue workload (the artifact-keeper/sparq migration's throughput investigation): the BGP operator was confirmed to touch every matching row (rows=800 at a queue depth of 800) before OrderBy/Slice could pick the top-1. Adds try_topk_orderby_indexed, tried first inside try_topk_orderby's OrderBy arm: for the narrow, syntactically-verified shape where the primary ORDER BY key is a bare variable bound as one pattern's object (constant predicate, variable subject -- the join "hub"), with every other pattern sharing that hub as its subject, it walks that pattern's value-sorted permutation scan directly (guarded to inline-integer objects only, the one case where ascending dictionary-id order is proven to equal ascending SPARQL value() order), joining each candidate against the other patterns via index-nested-loop, and stopping once a complete sort-key group has produced enough confirmed results. Declines (Ok(None)) at the first sign of anything outside that shape, so the existing materialize-then-select path is always the correctness fallback. Block sizing starts near row_budget (not the 1024-row CAPPED_SEED_BLOCK used for bare LIMIT) and grows geometrically: a first attempt at CAPPED_SEED_BLOCK-sized first blocks regressed moderate-n/small-k cases, since each candidate here pays a real per-pattern index-probe cost that a bulk merge join amortises away -- caught by re-profiling after the first cut, not assumed. Measured (synthetic claim-queue benchmark, single-node, in-memory graph): ~130us -> ~15-18us at a queue depth of 800, and latency stays ~flat (doubling ratio ~1.0-1.2x) instead of climbing toward 2x (O(n)) as queue depth grows to 3200. Verified via a new differential test suite (tests/topk_orderby_indexed_differential.rs) comparing this path's output against an independently-computed ground truth (fetch the full unordered relation, sort in test code, compare) across unique/tied priorities, both sort directions, multi-peer isolation, a fully-bound status filter, empty/ under-full queues, non-inline priority values (exercises the decline guard), an extra star pattern, and a 40-trial randomized sweep varying n, tie density, and k. A first cut had a real direction bug for DESC order (letting a naive block-slice-then-reverse silently return the LOWEST priorities first for N > 1024) -- caught by the block-escalation test, fixed by processing all blocks through a single logical best-first index regardless of scan direction, not by special-casing per direction. Full sparq-engine test suite (310 unit + all integration targets) and full-workspace clippy -D warnings pass; the one storage-differential failure encountered is confirmed pre-existing on unmodified main (compressed dict-spill build, unrelated to this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Root-causes a reported regression: an independent re-run of the S3
claim-contention benchmark (8-way concurrent claim on a shared queue,
the exact workload this optimization targets) found throughput DROPPED
after the previous commit -- sparq-serve claim throughput 1516->1090
ops/s (~27% lower), p50 5.3ms->8.0ms -- despite this function's own
isolated single-query profiling showing a large win (~130us->~15-18us).
Root cause, confirmed empirically (not assumed): that isolated profiling
used near-unique pseudo-random priorities. A real job-queue is much more
likely to use a small number of discrete priority TIERS (e.g. "urgent/
high/normal/low"), which produces large tie-groups. This function cannot
split a tie group across its early-stop boundary, so a low-cardinality
priority distribution forces it to fully process the group via
per-candidate index-nested-loop probes -- which have real per-probe
overhead (Store::choose + a binary search per pattern) that a bulk
merge-join amortises away over a whole scan. Verified directly: at a
queue depth of 1600, a tie-group of ~100 still beat the fallback (~47us
vs ~269us baseline), but ~320 was worse (~175-283us) and a single tie
covering the whole queue was far worse (~870us before this fix).
Also confirmed the OTHER hypothesis this regression could have had was
NOT the cause: instantiate_templates (the DELETE/INSERT WHERE path) calls
the exact same eval_select entry point a plain SELECT does, so the
optimization is reached identically from an UPDATE's WHERE clause -- this
was a real algorithmic cost under a specific data shape, not a dead code
path.
Fix: MAX_INDEXED_GROUP (128, scaled up for a larger row_budget) caps how
large a single escalation block may grow. Exceeding it declines
(Ok(None)) and defers to the fallback for the WHOLE query, rather than
partially applying the fast path -- the fallback's cost at that point is
only the pre-existing ~2-5x gap already accepted before this feature
existed, not a new regression. Re-measured after the fix: low/moderate
tie-group workloads keep the win (13-47us), high-tie-density workloads
now measure ~274-283us -- statistically indistinguishable from the
ORIGINAL unmodified baseline (~269us) at the same queue depth, i.e. the
regression is eliminated, not just reduced.
Adds large_tie_group_still_correct (tiers in {1,2,5,16} over n=600) to
the differential suite -- correctness only; the regression this fixes
was a performance issue, verified separately via profiling, not asserted
as a timing test to avoid flakiness. Full sparq-engine test suite and
full-workspace clippy -D warnings still pass (same pre-existing storage
flake as before, confirmed unrelated).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_orderby_indexed The previous commit (cap tie-group size in try_topk_orderby_indexed) fixed a real regression by declining above a flat MAX_INDEXED_GROUP=128 -- safe, but conservative: it gave up the optimization's benefit entirely for any moderately-tiered priority distribution, which is the realistic shape for a job-queue (a handful of priority tiers, not near-unique values). Investigated further to see how much of that gap was actually closeable rather than just declined around. Two real per-candidate costs were still present and are removed here: 1. Repeated permutation/bound resolution. Each (candidate, other-pattern) probe called `graph.store.scan(&probe_pat)` fresh, re-running `Store::choose`'s permutation selection every time even though the choice is IDENTICAL across all candidates for a given pattern (only the subject varies). Fix: resolve each other pattern's SUBJECT-sorted scan ONCE (`other_pats`, built before the block loop) and binary-search the resulting sorted slice per candidate instead -- O(log n) with no repeated setup. Falls back correctly (`Ok(None)`) when a store build can't provide a subject-sorted scan for a pattern (no PSO permutation under `compact-index`/wasm). 2. Per-candidate heap allocation from generality the common case doesn't need. The cartesian-expansion machinery (`Vec<SmallVec>`, cloning per combination, rebuilding the `cols` variable list from scratch every candidate) is only needed for a genuinely multi-valued predicate, which is rare (a real job-queue schema is single-valued: one status, one priority, one seq per task). Fix: hoist `cols`/the out-vars-to-columns mapping out of the per-candidate loop (computed once), and build each candidate's row directly into a stack-based SmallVec with zero heap allocation, falling back to explicit (and now genuinely rare) cartesian expansion only when a pattern actually yields more than one match for a given candidate. Re-measured (queue depth 1600 / 8000, varying priority-tier count = tie-group size): a tie-group of 320 dropped from ~146us to ~95us; 800 from ~367us (a regression vs. the ~269us fallback) to ~228us (now a clear win); at queue depth 8000, a tie-group of 4000 (half the queue) is ~1.3ms, roughly on par with the fallback's own cost at that n. This also changes what the SAFE threshold is: per-candidate cost is now roughly CONSTANT, while the fallback's bulk-join cost is roughly constant per `n` regardless of tie structure -- so the crossover is a FRACTION of `n` (empirically ~0.5-0.75), not a fixed absolute count. Replaces MAX_INDEXED_GROUP=128 with max_group = max(n/2, 256, row_budget*2): a tie-group covering the WHOLE candidate pool still correctly declines and matches the fallback's own cost (measured: ~272us at n=1600, matching the original ~269us baseline almost exactly -- confirming the decline path introduces no regression even in the worst case), while realistic partial-tier distributions now benefit up to a much larger group size than before. Also fixes two clippy::needless_range_loop lints from the rewrite (index-only for loops replaced with slice iteration). Verified: the full differential test suite (13 tests: unique/tied priorities, both directions, multi-peer isolation, status filtering, non-inline fallback, extra patterns, a 40-trial randomized sweep, and the large-tie-group regression test) passes unchanged -- this is a pure performance change, no new test cases needed since correctness was never in question, only cost. Full sparq-engine test suite and full-workspace clippy -D warnings pass (same pre-existing storage flake as before, confirmed unrelated in an earlier commit on this branch). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…exed
Root-causes a real, significant remaining cost this function's own
isolated-query profiling never surfaced: for a queue drained strictly in
priority order (claim highest-priority-first, the realistic shape), the
seed's priority-sorted scan accumulates an ever-growing CONTIGUOUS prefix
of already-claimed (status no longer "pending") rows at its head. Every
one of those still costs a real per-pattern probe before being rejected --
this function degrading toward the exact O(n) cost it exists to avoid,
just counting REJECTED candidates instead of matched ones.
Measured directly (single fresh graph, no chaining/compaction involved at
all, isolating this from every other hypothesis): query cost grew from
~13-22us at 0 already-claimed to ~200-270us at 400-1400 already-claimed
out of n=1600 -- an ~15-20x degradation purely as a function of
already-claimed prefix size. This also resolves an earlier open question
from this investigation: a "fresh load vs. many chained fork+compact
cycles" comparison had appeared to show cost driven by generation-chain
depth independent of data, but that test confounded chain depth with
prefix structure (the "fresh" comparison claimed a scattered subset by
seq order; the "chained" comparison claimed a contiguous top-priority
block, since real claims always take the highest remaining priority) --
there was never a separate compaction-driven phenomenon, just this same
skip-prefix cost measured under a different, confounded label.
Two guards, addressing two different aspects of the same problem:
1. An UPFRONT check, before touching a single candidate: each `other_pats`
scan's row count is the EXACT global cardinality of that pattern's own
constraint (e.g. how many tasks are still genuinely `pending`, overall).
If any other pattern is already meaningfully more selective than the
seed's own scan, the fallback's ordinary smallest-estimate seed
selection will pick THAT pattern as its seed and win outright with no
per-candidate cost at all -- decline immediately rather than discover
this the expensive way.
2. A reactive cumulative-failure counter as a safety net for skip-prefix
shapes the upfront check can't see (cardinality alone doesn't capture
every possible correlation between scan order and other-pattern
membership). Deliberately sized LARGE (matching the existing
`max_group` tie-group cap, not a small constant): an early attempt at a
tight budget (64) "solved" the worst case but broke the far more common
moderate case (a few hundred already-claimed) by bailing into a full
fallback when simply finishing the walk would have been far cheaper --
caught by re-testing across the whole range, not just the case being
fixed. Every failed probe before declining is pure waste stacked on top
of the fallback's own cost, so the budget trades a bounded, modest
worst-case overhead (~30-50% over a clean fallback, only when the
upfront check misses) for finishing the much more common moderate case
without ever falling back at all.
Re-measured after both guards (single-query, across the full
already-claimed range 0..1599 out of n=1600): consistently AT OR BELOW
the fallback's own cost everywhere, with strong wins in the common cases
(12-70us vs. a ~90-390us fallback) and modest wins even in the harder
cases (82-250us vs. 89-320us) -- no regression anywhere in the tested
range, unlike either guard alone.
End-to-end effect (8-way concurrent Writer claim benchmark, n=1600,
combined with the already-tuned compact_threshold=512 from the prior
config-only finding): throughput ~3,357-3,400 -> ~5,310 ops/s (+56-58%
over the pre-this-session baseline), narrowing the gap to Postgres's
SKIP LOCKED (~32,000-34,700 ops/s in the original S3 benchmark) from
~20-25x to roughly ~6x on this workload shape.
Adds large_already_claimed_prefix_still_correct (already_claimed in
{0,50,100,400,800,1200,1599} out of n=1600) to the differential suite.
Full sparq-engine test suite (310 unit + all integration targets, same
pre-existing unrelated storage flake as every prior commit on this
branch) and full-workspace clippy -D warnings pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-parse GraphApplier::apply calls sparq_engine::update_in_place(working, &String), which re-parses the update's FULL TEXT on every single commit -- for a caller resubmitting the SAME update template repeatedly with only a bound value changing (a job-queue claim, an upsert), that's pure waste every time. Measured directly, isolating the parse cost specifically: PreparedUpdate:: parse on a representative claim template costs ~13.5us, while PreparedUpdate::bind (structural placeholder substitution over the already-parsed algebra, never string concatenation) costs under 1us. End to end: raw-text update_in_place averages ~21.2us/update; parse-once + bind + update_in_place_prepared averages ~11.3us -- roughly half. Adds PreparedGraphApplier: an ApplyUpdates strategy identical to GraphApplier (same structural fork, same threshold-compaction seal policy) but with Update = sparq_engine::PreparedUpdate and apply() calling update_in_place_prepared instead. A caller parses its update template ONCE (e.g. at startup) and binds a fresh value per submission via PreparedUpdate::bind, instead of formatting a new SPARQL string and paying a fresh parse on the writer thread every time. footprint() returns Footprint::Barrier unconditionally: there's no parsed-algebra entry point into Footprint::of_sparql (text-only), and re-serialising the already-parsed update back to text just to re-parse it would defeat the whole point. This only affects the opt-in CommuteGroup commit granularity (degrades to one generation per update, still correct) -- orthogonal to this applier's goal, since the default Window granularity never calls footprint() at all. Gated behind a new opt-in `params` feature (forwarding to sparq-engine's already-dependency-free `params`), OFF by default -- the serving core is fully buildable without it, byte-identical to before. Measured end-to-end (8-way concurrent Writer claim benchmark, n=1600, compact_threshold=512): ~5,150-5,370 ops/s (raw text) -> ~5,730-5,840 ops/s (prepared), a consistent ~8-13% improvement across three runs. Combined with this session's other fixes (the indexed ORDER BY seed, its tie-group and skip-prefix corrections, and the compact_threshold tuning), overall throughput on this workload has gone from the pre-session baseline of ~3,357-3,400 ops/s to ~5,730-5,840 ops/s (+70-74%), narrowing the gap to Postgres's SKIP LOCKED (~32,000-34,700 ops/s, original S3 benchmark) from ~20-25x to roughly ~5.5x. Adds prepared_applier.rs (2 tests, only compiled under `params`): proves a bound PreparedUpdate applies correctly through the real sequenced writer (structural fork + delta-overlay + compaction, identical to GraphApplier's path), and that repeated submissions of the same parsed-once template drain a queue correctly across many generations with no duplicate/lost claims. Documents the feature in sparq-serve's README (kept in sync with Cargo.toml per its own house rule). Full sparq-serve test suite and clippy -D warnings pass in BOTH feature states (with and without `params`), and full-workspace build + clippy (default features) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Profiled the current best configuration end-to-end (8-way concurrent Writer, PreparedGraphApplier, compact_threshold=512) by instrumenting fork/apply/seal individually: sum(fork+apply+seal) was ~99% of wall-clock elapsed (the writer thread has essentially no idle/channel overhead left to chase), and apply_time -- dominated by the skip-prefix walk the prior two commits made safe but not free -- was the remaining cost, averaging ~161us despite the isolated single-candidate cost being far lower. Root cause: each per-candidate other-pattern check did `op.scan.rows.partition_point(|r| op.scan.to_spo(r)[0] < hub_id)` -- `to_spo` reconstructs the full permutation-order-independent [S,P,O] triple, and `partition_point`'s binary search calls that closure on EVERY comparison step (~log(m) times), even though the comparison only ever needs column 0. For a long skip-prefix (many candidates checked and rejected in a row -- exactly the case the prior two commits made safe, not fast), that's `O(log m)` wasted triple reconstructions per candidate per other-pattern, on top of the (already-fixed) per-call setup cost. Fix: precompute each other-pattern's subject ids into a plain `Vec<Id>` ONCE (when `other_pats` is built, not per candidate), in the same sorted order as the underlying scan. The per-candidate binary search then runs over a flat `&[Id]` with a trivial integer compare per step -- `to_spo` is only called after a MATCH is found (to extract an object value), i.e. at most `match_count` times (typically 1), never during the search itself. Re-measured (same 8-way concurrent Writer benchmark, n=1600, compact_threshold=512, PreparedGraphApplier): avg apply ~161us -> ~125-137us (-15 to -22%), throughput ~5,850 ops/s -> ~6,800-7,440 ops/s (+16-27%, consistent across three runs). Combined with every fix this session, overall throughput has gone from the pre-session baseline of ~3,357-3,400 ops/s to ~6,800-7,440 ops/s -- more than DOUBLED -- narrowing the gap to Postgres's SKIP LOCKED (~32,000-34,700 ops/s, original S3 benchmark) from ~20-25x to roughly ~4.6-5x. No behavior change, no new test cases needed: the full differential suite (14 tests) already covers this code path and passes unchanged; this is a pure internal representation change (search over `Vec<Id>` instead of `Scan::rows` via `to_spo`) with byte-identical output. Full sparq-engine test suite and full-workspace clippy -D warnings pass (same pre-existing unrelated storage flake as every prior commit on this branch). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…f number Two preflight findings from scripts/preflight.py, fixed before opening the PR: 1. The params/PreparedGraphApplier bullet added in 7a89065 pushed the README past the 120-line template cap (the file was already exactly at 120 before that change, with zero margin) -- trimmed it and tightened an adjacent blockquote to make room, no content lost. 2. That bullet also hard-coded a specific measured number (~13.5us of ~21.2us, ~63%) directly in the README -- against this repo's own "no hard-coded performance numbers in markdown" rule (CLAUDE.md, and the PR template's own checklist item). Replaced with the qualitative claim the number was illustrating (avoids the per-commit re-parse), which stays true regardless of hardware/measurement drift. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every existing test in this file checks CORRECTNESS -- which the fallback
path (eval_modified + order_bindings) satisfies on its own. None of them
would fail if try_topk_orderby_indexed were deleted entirely: the whole
feature could regress to a no-op and the suite would stay green. Per
scripts/preflight.py's mandatory (non-automatable) obligation to mutate
the headline guard and confirm it's not vacuous, verified directly.
Adds fast_path_actually_engages_not_just_correct, using the engine's own
EXPLAIN ANALYZE instrumentation. First attempt checked for the ABSENCE of
any "BGP [" substring in the output -- this was itself vacuous: the
"Plan:" section is a static description of the general planner's choice
and always names a "BGP [...]" step regardless of which path actually
executes, so that assertion failed even with the fast path correctly
firing. Fixed to check the "Execution trace" section specifically: it
only emits a "BGP [binary GOO] (... patterns ...) rows=N" line when the
general BGP evaluator was ACTUALLY EXECUTED (materializing and reporting
the touched row count) -- the indexed fast path bypasses that evaluator
entirely, so the line's presence is a genuine, structural signal of which
path ran, not a name/type/marker string.
Verified non-vacuous by mutation, per the script's explicit instruction
("do not reason about it; execute it"): temporarily forced
try_topk_orderby_indexed to return Ok(None) unconditionally (disabling
it), re-ran the suite, and confirmed EXACTLY this new test went red while
all 14 other (correctness-only) tests stayed green -- proving both that
this guard actually detects the fast path being disabled, and that the
existing correctness tests genuinely do not (by design: they check
answers, not which code path produced them). Reverted the mutation
immediately after.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
scripts/check-feature-test-execution.py --check caught this before it became a silent gap: PreparedGraphApplier's tests/prepared_applier.rs is behind the default-OFF params feature, so ci.yml's default-feature nextest run compiles it EMPTY and would have run ZERO of its tests -- exactly the class of gap #1171 and the sq-vya1 tier ratchet exist to catch (this is the same pattern the existing change-stream/change-sink legs in this fragment close for their own features). Adds a leg to .github/feature-matrix.d/sparq-serve.yml: clippy --all- targets -D warnings with params ON, plus cargo test (both prepared_applier.rs tests). Per this fragment's own documented scope rule, adding a leg changes the emitted leg-name set, which is a two-file change -- updated scripts/tests/feature-matrix-legnames.golden.txt in the same commit, in the correct sorted position (verified: `diff <(python3 scripts/assemble-feature-matrix.py --names) scripts/tests/feature-matrix-legnames.golden.txt` matches). Verified locally, matching exactly what CI will run for this leg: `cargo test -p sparq-serve --features params --test prepared_applier` (2 passed) and `cargo clippy -p sparq-serve --all-targets --features params -- -D warnings` (clean). Also re-ran scripts/check-feature-test-execution.py --check (now OK, previously flagged this exact gap) and scripts/feature-matrix-tiers.py --enforce (187 legs, 0 violations). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes the two regressions reported in #6465 against this PR: - Repeated variables: decline when a pattern repeats a variable (has_intra_triple_repeated_var), so ?x <p> ?x is no longer answered without the equality check. - Intermediate budget: decline whenever a query budget is active, so the existing evaluator keeps its row/byte accounting and cancellation. Also keeps multi-valued probes on the existing evaluator, rejects an oversized leading tie before preparing probes, prepares later probes only when a candidate reaches them, and applies the cardinality admission rule per probe. Recovered from the maintainer-side Codex review branches (codex/perf-indexed-topk-recovery, codex/perf-topk-lazy-probes) and adds the feature-off declaration. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR
13 tasks done
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR
Collaborator
Author
🔎 Codex reviewer —
|
…riables Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR
Collaborator
Author
|
Fixed in bbb3622. The indexed top-k path now declines when any pattern holds a triple term (it reuses Generated by Claude Code |
Collaborator
Author
🔎 Codex reviewer —
|
Collaborator
Author
|
Local ci-fast gate (GitHub Actions incident; Jesse approved local-gate merges at 20:19Z). Tested PR head Main was re-checked immediately before merging. Generated by Claude Code |
7 of 10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Jesse · project thread
Before: #5983, from @KamiQuasi (Luke Dary), adds an indexed top-k path for star-shaped
ORDER BY … LIMITqueries, atxnretry helper, and aPreparedGraphApplierfor claim-queue workloads. #6465 reported two regressions in it:SELECT ?x WHERE { ?x <urn:p> ?x } ORDER BY ?x LIMIT 1returned a row it must not.max_rowssucceeded instead of raising the intermediate-row limit error.After: this PR carries #5983's commits unchanged, merged with current main, plus one fix commit. In that commit the indexed path declines in four more cases, which then use the existing evaluator:
It also prepares later probes only when a candidate reaches them, and applies the cardinality admission rule per probe. The fix is recovered from the maintainer-side review branches
codex/perf-indexed-topk-recoveryandcodex/perf-topk-lazy-probes; the harness-onlybench/indexed-topkfrom those branches is left out.Closes #6465. Supersedes #5983: the bot can't push to the contributor's fork, so the commits are carried here with their authorship intact.
How. The fix commit changes
try_topk_orderby_indexedincrates/sparq-engine/src/exec.rsand extendstests/topk_orderby_indexed_differential.rs.Verified locally:
topk_orderbyandorderby_limit_zeropass.txnlib tests pass, as do thesparq-serve --features paramstests.-D warningsis clean, default and withtxn,params.scripts/preflight.pypasses.The PR makes no performance claim. Because the path now declines under any active budget, it does not fire on the HTTP server's deadline-budgeted queries.
Benchmark
Local
sparq-cli bench, operators suite (2,000 entities, 16k triples), release-fast binaries of main03775daee2and this PR. Best of 5 iterations per round, minimum over 5 interleaved rounds; row counts match on every query. The indexed top-k path takes q17_orderby_limit_offset from 300 µs to 19 µs, about 16x faster. Every other query is within noise, and the geomean is 0.897.🤖 Generated with Claude Code
https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR
Generated by Claude Code
Generated by Claude Code