Refactor store and query logic for performance improvements - #271
Merged
Merged
Conversation
`ScanRequest::count_storage_rows` added both row counters and had no caller anywhere in the workspace. Its presence read as the mechanism by which a request records rows, so attaching counters to a scan looked like it would start counting them — and it did not. That cost real time: a MERGE key scan was switched to the counted entry point expecting rows and got none, which took a debug print at the call site to see. Wiring it instead would double-count. `columnar_scan::merge_lance_and_l0` already adds `lance_rows + l0_rows` from the batch after the request returns, so every Lance row would land twice. Counting rows in the request means moving that, which changes every `rows_scanned` in the codebase for no consumer currently asking. Deleted, and `with_counters` now says what counters on a request do buy — index scans, comparisons, iops, branch scans — and what they do not. The sibling `count_branch_scan` is live and untouched.
`cypher_size_scalar`'s tagged-value arm handled lists, strings and maps and fell to `Null` for everything else, so `length()` over a tagged number or boolean answered null. The outer match already errored on a type it did not recognise; the inner one now agrees with it. Null was the worst available answer. It is indistinguishable from "an empty collection" and from "this encoding was not understood", and the second reading is not hypothetical — it is exactly what a missing `Value::Path` arm produced in this same match until `length(p)` was caught measuring a JSON object's key count. A type error would have surfaced that the first time anyone called it. Null in, null out is kept: the Cypher convention for a function applied to a missing value, and the one case where a null answer is an answer. The test covers both halves of the function deliberately. A native `Int64` property never reaches the arm changed here — it takes the outer match, which already errored — so only a tagged value, such as a `JSON` column holding a number, exercises this. Asserting both is what stops the two halves drifting apart again, which is how the original gap survived. BREAKING CHANGE: `length(x)` / `size(x)` raise a type error where `x` is a tagged number or boolean, instead of returning null. A query relying on that null was relying on a value it could not distinguish from an empty collection. Same family as the `nodes()` / `relationships()` tightening already on main. Tests: 2945 uni-db, 1020 across uni-store and uni-query-functions, 3925 openCypher TCK — which specifies type-error behaviour for these functions and is why tightening is safe. Shown to discriminate: restoring the catch-all fails the new test.
Both are wall-clock tests that passed in isolation and failed inside a full `cargo nextest run -p uni-db`. The repository asks for that constraint to be recorded at the test site, where the next person to see it red will be looking. `fork_point_atomic_under_concurrent_parent_writes_*` races a fork creation against a writer loop on a 4-worker runtime, so it needs those workers scheduled. Observed once at 34s under a full run against 1.5s alone, passing on the next full run of the same tree. Documented on both the async- and sync-flush twins; no behaviour change. `issue_267_node_key_scaling` needed more than a note. Its 4x guard was calibrated against arms measuring 0.72s and 0.49s; they now measure a fifth of that, where fixed overhead is a large share of each arm and the ratio swings with it rather than with the work — 3.0x alone, past 4x under load. It was failing on its own noise, which is worse than no guard: a test that cries wolf gets re-run rather than read. Each arm is now measured twice with the faster kept, since the first run of each pays plan construction the second does not, and the threshold moves to 6x. Measured at 3.1-3.3x across repeated runs, against the 12x release / 41.9x debug the defect produced — so the gate still catches what it was built for. Worth separating: this guard *was* verified to discriminate when written — 41.9x with the fix reverted, 1.5x with it, both true. What was not checked is whether the margin stayed stable as the absolute times shrank. Proving a test can fail for the right reason says nothing about whether it also fails for the wrong one.
…g out `get_batch_edge_props` asked L0 for each EID's type and, if a single EID was unknown to it, scanned every edge type in the schema. L0 is empty on a reloaded or compacted store, so that fallback was the normal path rather than the exceptional one: on LDBC IC5's HAS_MEMBER clause at SF1 all 195 calls took it, scanning all 15 edge types for 31.7 s — 20% of the clause. Three of the four production callers already pass the `edge_types` hint added earlier. The fourth, `PathPropertyCache::prefetch`, cannot: its EIDs come from already-materialised paths that carry no type column, so there is no hint to thread through. Rather than special-case that caller, this fixes the fallback it lands in, which fixes every unhinted caller at once. `MainEdgeDataset::find_types_by_eids_counted` resolves the misses in one indexed pass over `_eid` and `type` — never `props_json`, since the point is to choose which tables to read and paying a blob decode here would defeat it. An EID that neither tier knows still fans out: narrowing on a partial answer would scan too few tables and silently return no properties for the edges left out, which is a wrong answer where the fan-out is only a slow one. The resolver runs no MVCC contest, deliberately. An edge's type is fixed for the life of its EID, so every row for it — live, superseded, or tombstoned — agrees, and ranking could only narrow the result in the one direction that is unsafe. Measured on the test fixture: an unhinted read over a cold L0 scans 2 tables against the previous 6 over 6 declared types, the resolution pass included. At LDBC's 15 types the ratio is 2 against 15. Verified discriminating: forcing the fallback back to the fan-out fails `a_cold_l0_read_no_longer_scans_every_edge_type` at 7 scans against 6. The direct probe `a_cold_l0_resolves_edge_types_from_main_edges` is separate and load-bearing — if main_edges held no row for a flushed edge the resolver would return an empty map and every contrast test would still pass, having merely paid one extra scan on the way to the same fan-out. 731/731 uni-store and 3871/3871 uni-db/uni-query/uni-algo green. Fixes rustic-ai#222
The BFS issued one adjacency scan per (vertex, label) and one delta scan per vertex, on every hop, so storage round-trips scaled with the frontier. Both batched primitives it needed already existed and had **zero callers anywhere in the workspace**: `AdjacencyDataset::read_adjacency_backend_batch` and `DeltaDataset::read_deltas_batch`. They were written and never wired up. The hop now resolves its whole frontier first, then reads once per (edge type, label) for adjacency and once per edge type for delta, chunked at 10 000 VIDs to keep the scalar index in play at any frontier size — the same bound the vertex and edge read paths already use. Two semantics are preserved deliberately: * The per-vertex `break` on the first label with adjacency data becomes a carry-forward of the vertices no earlier label answered for, so the first label still wins and a vertex is never counted twice. * Marking the hop's vertices visited up front is what makes the batch possible. It does not change the resulting graph: `visited` gates only `next_frontier` insertion, never an edge, and a neighbour it suppresses is one this hop already covers — the old code queued it and then skipped it on arrival. Measured on the test fixture: a frontier of 8 issued 8 scans and one of 16 issued 16; both now issue 1. Verified discriminating: against the previous per-vertex BFS the new test fails with exactly that 8-and-16 growth. The observable is a counting `StorageBackend` decorator rather than `QueryCounters`, because these two read paths build their `ScanRequest`s without counters — the query-level counter reports zero however many round-trips happen, so it could not tell the two shapes apart. 732/732 uni-store and 3871/3871 uni-db/uni-query/uni-algo green. Refs rustic-ai#220
rustic-ai#237 asks for a vertex-side probe "built before the fix, so it can fail first", and is explicit that rustic-ai#221's edge constants must not be copied across. This is that probe for the remaining site: `find_edges_by_type_names`'s endpoint-vid arm, which picks between a chunked `IN (...)` lookup and a whole-type scan by which `match` arm the caller lands in, with `prefers_full_scan` sitting in the same file and never consulted. Both arms are made to return the same edges over the same type, so the only difference is read strategy, and K is swept across three orders of magnitude so a flat result would indicate the probe is measuring nothing. Measured at LDBC SF1 on HAS_MEMBER (1 611 869 edges, 79 470 distinct src vids), min-of-3, release: the lookup arm is linear in K (21 ms at K=100, 389 ms at 10 000, 2 485 ms at 79 000) and the scan arm is flat at 840 ms. They cross between K = 20 000 and K = 30 000 — about 28% of the type's distinct sources. At full breadth the lookup the code always takes is 3.0x slower than the scan it never considers. The rule does not follow from this yet, and the probe says why rather than fitting a constant to K. The decision needs a denominator: `count_rows` is metadata-only but counts every edge type (~17.3M rows), while this scan arm is bounded by the `type` predicate, and asking for a type's row count is a filtered count, which scans. That is rustic-ai#260 — no cached cardinality statistic exists, so a selectivity rule has nowhere to read its denominator. So this lands as the measured consumer that makes rustic-ai#260 concrete: the crossover is now known, and what remains is the statistic to compare against. Fitting to K alone would reproduce the mistake rustic-ai#221's own rustdoc concedes. Refs rustic-ai#237, rustic-ai#260
Six of the eight open sites in the class. Each replaces a loop issuing one storage round-trip per item with a batched read; five call a primitive that already existed, and one needed a new one. * `read.rs::verify_and_filter_candidates` — one `get_batch_vertex_props` for the candidate set. The hazard here was that a live vertex carrying no properties might be absent from the batched map and so be dropped from a filter, turning a slow path into a wrong one. Measured before the swap: both forms return it, the singular's `None` meaning only "in neither layer". * `read.rs` CSV and JSON/parquet export, four arms — chunked at 10 000 rather than batched whole. Reading a whole label in one call would remove the round-trips but hold every row's properties at once, which would stop the CSV path streaming; chunking gets the batched read with residency bounded. * `writer.rs` relabel flush — two reads per vid become two reads for the set, with the version bounds each carried before. * `writer.rs` orphaned-tombstone label recovery — one `find_batch_labels_by_vids` for the orphan set. The error still propagates, which is the rustic-ai#233 property that comment guards. * `writer.rs` ext_id uniqueness — new `MainVertexDataset::find_by_ext_ids_counted`, a chunked `ext_id IN (...)` with the singular form's per-key MVCC ranking: highest `_version` wins with tombstones in the contest, and a tombstoned winner is absent from the map. Ordering is unchanged, so the batch still reports the first offending index. * `locy_fixpoint.rs` neighbour aggregator — the adjacency walk is in-memory, so hoisting it and reading all neighbours' property once is what removes the round-trips. Site 4, the CRDT pre-merge, is deliberately NOT changed. rustic-ai#220 lists `get_batch_vertex_props_for_label` as its batched primitive, but the two are not equivalent for the keys that site reads: for a CRDT property the singular `get_vertex_prop_with_ctx` takes a different path entirely (`accumulate_crdt_from_l0` then `finalize_crdt_lookup`, merging across every L0 layer), while the batched form only runs `normalize_crdt_properties`, which fixes JSON shape rather than merging. That site reads exclusively CRDT keys, so swapping it would change merge semantics in the area with a lost-update history. It needs an equivalence test first. Measured: a batch insert of 8 issued 9 storage scans and one of 16 issued 17; both now issue the same count. Verified discriminating — restoring the per-vertex probe fails the new test with exactly that growth. 1566/1566 uni-store and uni-query, 3329/3329 uni-db/uni-algo/uni-query-functions. `fork_point_atomic_under_concurrent_parent_writes_sync_flush` failed once at 32.3s in a loaded full run; it passes in isolation at 2.0s, passed 10/10 on repeat, and the same tree then passed the same full command. That matches the load-sensitivity documented at the test site (34s against 1.5s), and the control is the clean re-run of this tree rather than the documentation. Refs rustic-ai#220
Site 4 is the last of rustic-ai#220's eight, and the batched primitive the issue lists for it is not equivalent to what it replaces. `get_batch_vertex_props_for_label` folds an L0 overlay over storage with `entry(k).or_insert(v)` — the overlay wins the key outright — while the singular `get_vertex_prop_with_ctx` branches on CRDT-ness and *merges* the two. Measured on a `GCounter` with `actor1=10` in storage and `actor2=20` in an overlay that does not subsume it: the batched form answers 20, the singular one 30. That is read by `insert_vertices_batch`'s CRDT pre-merge, which merges the incoming batch value against the existing one. Merging against a value with storage's replica dropped writes a counter that has gone backwards — the lost update the OCC work closed once already. So the swap rustic-ai#220 proposes is a silent correctness regression, and the issue's table is wrong on this row. `PropertyManager::get_batch_vertex_crdt_props` batches the half that actually costs round-trips — the storage read, one call for every vid — and keeps the merge exact, because the L0 accumulation is a walk over in-memory buffers that never touched storage to begin with. Two tests in `bugs::issue_220_crdt_reader_equivalence`, and the pair is the point: the first asserts the readers agree on the state the ordinary write path produces, and is kept precisely because it is NOT evidence of interchangeability. `insert_vertex_with_labels` merges the incoming CRDT against storage at write time, so L0 there holds both replicas and both readers answer from the overlay alone — a reader ignoring storage entirely would pass it too. Its twin builds the partial overlay by hand and is the one with teeth. The end-to-end test took two tries to become discriminating, and its rustdoc records both corrections. Seeding storage and flushing is not enough, because a flush empties L0 and the readers then agree; and reading the property back through the singular reader masks the defect, because it re-merges storage on the way out. It therefore injects a partial overlay into the live L0 and asserts on the value the pre-merge *wrote*. With the defective reader wired in it fails at 50 against 60; written the first way it passed. 1569/1569 uni-store and uni-query, 3329/3329 uni-db/uni-algo/uni-query-functions. Refs rustic-ai#220
…wait Every decision that wanted to know how big a table is was fitted to a constant, because the primitive that can answer — `count_rows` — is async and plan-time paths are not. `Planner::estimate_costs` took `_plan`, ignored it, and returned `estimated_rows: 100.0`. `CardinalityCache` caches the flushed count per vertex label and per edge type, behind `StorageManager::cached_row_count`, which is synchronous. The counting was never the hard part; the four that were: **Readable without await.** Only the flushed half is cached, because only that half is expensive. The L0 half is a walk over an in-memory index, read live on every call. **L0 inclusion.** Reading live is what keeps an unflushed write visible. `api/schema.rs` records a `count: 0` returned for a label whose rows were all L0-resident — "a silent wrong answer, and the reason a Python assertion on this value was once weakened rather than fixed". A test asserts the L0 case specifically, since that is the one where being wrong is silent. **The contract is an upper bound, and says so.** The flushed count and the L0 vid set are each exact; their overlap is not knowable synchronously, so a vertex updated in place is counted twice and an L0 tombstone over a flushed row is not subtracted. The direction is chosen, not accepted: every consumer uses the count as a denominator for "is my request big relative to this table", so over-estimating biases toward the indexed-lookup arm, which costs the request rather than the table. **Views that are not the live tip decline.** Fork-scoped and pinned readers get `None`, and `None` is documented as "no answer", never as zero. A fork's refresh also declines, so its count can never be written into the shared cache and read back as primary's. Invalidation is on flush and compaction, in `flush_finalize_body` — the shared body of the sync and async paths, since a hook on either alone would leave the other serving a stale count — and at the head of `compact_all`, where a partial run must still leave nothing stale. Commit needs no hook at all, because the half a commit changes is the half that is never cached; there is a test for that. Two consumers, one of them rustic-ai#237: * `StorageManager::find_edges_by_type_names_counted` now chooses between one pass over the edge type and chunked endpoint lookups on the cached count, where it used to choose by which `match` arm the caller landed in. The crossover is the one measured in `examples/endpoint_arm_probe.rs`: at LDBC SF1 over HAS_MEMBER the lookup arm is linear (21 ms at K=100, 2 485 ms at K=79 000) against a flat 840 ms scan, crossing near K=25 000 against 1 611 869 rows. * `estimate_costs` sums the cached counts of the labels its plan scans. A total of zero is treated as no information rather than reported as zero rows, since the planner holds no L0 handle and an all-unflushed label caches a legitimate zero. rustic-ai#237's other two sites are deliberately untouched. Its own probe found no scan-versus-lookup crossover on the vertex path — the cost there was duplicate targets, already fixed by `DEDUP_TABLE_RATIO` — and records that `vid_lookup_join` chunks a set that is already distinct and needs its own measurement. Fitting those constants without one is what this issue exists to stop. Verified discriminating: with the arm choice forced off, the new endpoint test fails at 3 scans against 3. 1580/1580 uni-store and uni-query, 3329/3329 uni-db/uni-algo/uni-query-functions. Fixes rustic-ai#260
The last two of rustic-ai#237's three sites. Both chunk a vid list at a fixed `MAX_VIDS_PER_CHUNK`, and `scan.rs` ended its comment block with, verbatim, "A selectivity-aware choice would beat a fixed constant". They now consult rustic-ai#260's row count instead. The probe came first, as rustic-ai#237 asks, and it had to be rebuilt once. Run against the LDBC SF1 store it reported that one scan beat the chunked arm at every K, including K=10 out of 2M rows — an artifact: `list_indexes` returns **zero** indexes for that store's `vertices_Person` and `vertices_Comment`, so its "lookup" arm was a sequential scan wearing an `IN` predicate, which is strictly worse than a bare scan and is not the shape production has. A store written through the ordinary path gets `_vid`, `_uid` and `ext_id` BTrees from `VertexDataset::ensure_default_indexes` at flush. The probe now builds its own fixture and refuses to report unless `index_comparisons` proves the lookup arm used an index. (That an SF1 store has no vertex indexes at all wants its own look; it is not this issue.) Measured, release, min-of-3: rows scan crossover K K/N 30 000 1.7 ms ~280 ~0.9% 300 000 4.0 ms ~800 ~0.27% 1 000 000 7.0 ms ~1 100 ~0.11% Two results. A crossover exists at every size, since the scan arm is flat in K and the lookup arm linear. And **K/N is not stable** — it falls about 8x across a 33x range of rows, because a columnar scan grows far slower than linearly. That is exactly what rustic-ai#237 predicted when it refused to let rustic-ai#221's edge-side ratio be copied across, and it is why the threshold is not derived from these crossovers. The crossover is the wrong threshold anyway, because time is not the only axis and the two disagree. Just past it the scan arm is barely faster while materialising the whole table to return a few hundred rows: at K=1 000 of 1 000 000 it saves 0.7 ms and reads 1 000x the rows. Chunking is here to bound peak residency — 60 000 vids from a 300k-row table went 815 MiB to 226 MiB — and trading that away for sub-millisecond wins would undo it. So the switch waits for 25% of the table, where the memory is comparable anyway and the win is large: 4x the rows for ~50x the speed at 300k (208 ms to 4.0 ms), 2x the rows for ~100x at 50%. Below that the chunked arm keeps its bound. An unknown size — a fork, a pinned view, an uncounted table — keeps the chunked arm too, since not knowing a table's size is not a reason to read all of it. `VidLookupJoinExec` is safe to switch because its probe batch is consumed through a `_vid` index that *build* rows drive: a probe row whose vid is not wanted is never looked up. Its reservations are unchanged and still measure the real batch, so the larger residency this arm accepts is accounted rather than hidden. Confirmed the new arm actually executes rather than passing by never running: instrumented, it fires 6 times across the traversal tests and at k=n in the vid-lookup-join tests — the 100% selectivity case `scan.rs`'s own comment named as the one where a full scan wins. 1583/1583 uni-store and uni-query, 3329/3329 uni-db/uni-algo/uni-query-functions. Fixes rustic-ai#237
The last of rustic-ai#220's sites, and the measurement moved both the fix and its size. rustic-ai#220 classes this one as structural: `evaluate_expr` falls back to a single-key property read when the in-memory `Node` lacks the key, and it runs per row, so the fix "means a prefetch pass, not a call-site change". Instrumenting the fallback first showed where it actually fires, and it is narrower than that framing suggests. Ordinary `MATCH ... WHERE ... RETURN` never reaches it — those plans run through `df_graph` — and the whole `uni-db` suite produced ~45 occurrences. One shape accounts for nearly all of them: `MERGE (n:E {k: ...}) ON MATCH SET n.p = n.p + x` over `UNWIND`, the accumulate ingest pattern, at 617 reads in a single test subset. No prefetch pass was needed, because the prefetch already exists. The per-statement persisted lookup (`merge_lookup_persisted_batch`) reads every matched vid's properties up front, and `merge_prefetch` carries them into `execute_merge_row_indexed`. The general match path then bound the node with `HashMap::new()` anyway — "minimal binding so ON MATCH SET resolves the node by `_vid`" — so every `n.p` in the SET expression missed the in-memory map and went to storage. The rebind *after* the SET already made exactly the right call for RETURN fidelity; this hoists the same call above the SET so the expression can see the properties too. A prefetch miss behaves as it does in that rebind: one full-property read rather than one per property touched. `merge_label_prefetch_safe` still excludes CRDT labels, so the CRDT read-modify-write path is untouched. Measured: 617 per-row storage reads to 0 on the subset, and 20 000 rows of `ON MATCH SET n.freq = n.freq + 1` against a flushed store from 473-492 ms to 447-451 ms — about 6%, reproducible across runs with non-overlapping ranges. Worth having and worth being plain about: the per-row read was a good deal cheaper than this issue's framing implies, and MERGE's other work dominates. Four fallbacks remain, all `ON CREATE SET n.created = coalesce(n.created, 0) + 1`. That one is correct as it stands — a node being created has no persisted row to prefetch, and `coalesce` is asking precisely because the value may not exist. 1583/1583 uni-store and uni-query, 3329/3329 uni-db/uni-algo/uni-query-functions. Closes rustic-ai#220
`VidLookupJoinExec::children()` returns the build side only — the probe is
driven through a `GraphScanExec` helper rather than `execute()` — and
`collect_plan_metrics` recurses strictly through `children()`. So profiling a
query that used this operator showed a join with one input and no scan beneath
it, omitting precisely the side the operator exists to make cheaper. Measured
before the fix, the operator list was:
["GraphScanExec", "VidLookupJoinExec", "AggregateExec", "ProjectionExec", ...]
one scan, and it was the build.
Two mechanisms had to be fixed together, as the issue notes: being absent from
the walk, and never constructing a `BaselineMetrics` so there would have been
nothing to report even if it were reached. The join now owns a second metrics
set for the probe, times the probe materialisation and records its output rows,
and threads a metric sink into `execute_with_vid_filter` / `execute_all` so the
probe's index consultation is counted too. `collect_plan_metrics` reads that set
back and emits it as its own entry, before the join, keeping the post-order the
rest of the walk produces. The sets are kept separate deliberately: folding the
probe's rows into the join's would make the join look like it emitted rows it
did not.
The profile now reads:
[("GraphScanExec", 50, idx 0), ("GraphScanExec", 50, idx 1),
("VidLookupJoinExec", 50, -), ("AggregateExec", 1, -), ...]
This also restores the operator-activation gate, which asserts operators through
`ProfileOutput::runtime_stats` — the same `children()` walk — and so could not
see anything beneath a probe.
The test has to work to be discriminating, and the issue says why: the build side
is a `GraphScanExec` too, so "assert a GraphScanExec is present" passed before the
fix. It demands two scans, and identifies the probe by the asymmetry that cannot
be faked by a spliced duplicate of the build — exactly one of the two consults an
index. Row counts cannot carry that weight here, since both sides read the same
number of rows. Verified discriminating: with the splice disabled it fails at one
scan.
What this does NOT fix, recorded at `children()` rather than left implicit:
optimizer visibility. DataFusion's rules also recurse through `children()`, so the
probe subtree is skipped by any tree-walking rule. That is harmless rather than
correct today — the planner's guard keeps the probe a bare `GraphScanExec`, a leaf
with no work for a rule to do — and stops being harmless if that guard ever admits
a non-leaf probe. Routing the probe through `execute()` with a dynamic filter is
the larger change the issue names as the long-term shape.
1583/1583 uni-store and uni-query, 3330/3330 uni-db/uni-algo/uni-query-functions.
Fixes rustic-ai#179
Completes rustic-ai#179. The previous commit took the issue's option 2 — record the probe's metrics separately and splice them into the collected vector — which restored `PROFILE` and the operator-activation gate but left the probe outside `children()`, so DataFusion's tree-walking rules still skipped the subtree. That residual is now closed rather than documented: this is the issue's option 1, the shape it names as correct. The probe was driven through a bespoke `execute_with_vid_filter` helper because its vid set is not known until the build side is materialised, and that is exactly why it could not be a child. `DynamicVidFilter` is a slot the join publishes into immediately before executing the probe, so the scan can resolve its `_vid` restriction at `execute()` time instead of plan time. With that, `children()` returns both sides, `with_new_children` takes two, and the probe runs through the ordinary `ExecutionPlan` API. Everything that had to be special-cased falls out: * The probe's own `BaselineMetrics` and `index_consulted` counter register on its own node, so the splice in `collect_plan_metrics` and the join's second metrics set are both deleted. * `execute_with_vid_filter` and `execute_all` are gone; their callers are the ordinary `execute()` path. * Chunking stays in the join, deliberately. `GraphScanStream` hands its whole vid list to one scan call, so a cap-busting build set must be split before it is published or the `_vid` IN-list would exceed the bound that keeps the scalar index earning its keep. The high-selectivity arm (rustic-ai#237) publishes an empty slot instead, which the scan reads as "no restriction". The SSI guard in the planner is unchanged and its comment is corrected. It still refuses to peek through a `ReadSetRecordingExec`, but not for the reason recorded: the probe no longer bypasses the wrapper at execute time, and what would lose the reads is *discarding* the wrapper at plan time, since `try_new` requires a bare `GraphScanExec`. The profile test from the previous commit needs no change and still discriminates — with `children()` reverted to the build side alone it fails at one scan. It identifies the probe by index consultation rather than row count, because both sides read the same number of rows and a duplicate of the build would satisfy any row-based assertion. 1583/1583 uni-store and uni-query, 3330/3330 uni-db/uni-algo/uni-query-functions, including the SSI, plan-shape and dqp-lever suites. Marked breaking for the `with_new_children` arity change on a public operator. Refs rustic-ai#179
`MAX_UNPROVEN` goes 10 -> 0. Every operator in the registry is now backed by an assertion that would go red if it stopped being emitted, and no row is left in `Unproven`. Four of the ten were plain gaps and needed only a proof: `CatalogVertexScanExec` (the virtual-label fixture existed and asserted rows, never shape), `DerivedScanExec`, `BestByExec`, and `StorageScanExec`. Six needed something built first. **Locy had no plan-shape accessor.** `locy_plan_ops` flattens `LocyProfileOutput`'s strata -> rules -> iterations -> operators, which are produced by the same `collect_plan_metrics` walk Cypher uses over each rule's re-planned clause body. `DerivedScanExec`, `BestByExec` and `LocyModelInvokeExec` were reachable through it immediately. **`LocyBuilder::profile` could not compile what `run()` executes.** It compiles for its explain half and did so through the no-config entry point, which hardcodes `neural_predicates_preview = false` — so it rejected every `CREATE MODEL` program, which is the only way to reach `LocyModelInvokeExec`. `Session::compile_locy_with_config` fixes it. Note `session.rs` already called this exact hazard out for the monotonicity oracle — "a task-local would make `explain()` reject a program `run()` accepts" — and solved it there; the config had the same problem and had not been looked at. **The post-fixpoint chain reported nothing.** `FoldExec` and `PriorityExec` are never lowered into a clause body — folding in the body would double-apply the aggregate, and `clause.priority` stays a scalar — so they are assembled in the chain and were in no collected plan. The chain now reports its operators into the rule's profile, recorded at each of its execution points because every stage replaces the plan with an in-memory source over its own output and only the stage that ran a tree can report it. The registry had prescribed exactly this. **`FixpointExec` belonged to the stratum, not to a rule.** It drives every rule in a recursive stratum and is a child of nothing, so it could never appear in a rule's operator list. `LocyStratumProfile` grows an `operators` field and it is reported there. Attributing it to one rule would have been a misstatement. **`CatalogEdgeScanExec`'s row was stale.** It said "reachable only in principle" pending a native source/target label resolution the MVP did not cover, and said to prove it by finishing that resolution rather than testing the planner of the day. That resolution had since landed — `plugin_mid_pattern_virtual.rs` is that work — and nobody revisited the row. Measured: both the native-source and virtual-source shapes emit it. `LocyProgramExec` is the one row that is weaker than the rest and says so: it reaches neither profile surface, so it is proven by planning the logical node in `uni-query` and asserting `plan.name()`. That proves emission, not execution. Every operator gets a negative twin. Two are load-bearing rather than routine: the fixpoint one, because a stratum operator list populated unconditionally would satisfy the positive assertion for every program; and the catalog pair, chosen by schema resolution rather than by query text. The gate learned `assert_locy_plan_uses` — a Locy-only operator can be proven no other way, since it never appears in a Cypher `ProfileOutput`. Verified discriminating: with the post-fixpoint and stratum recording disabled, the `FoldExec`, `PriorityExec` and `FixpointExec` proofs fail while their twins still pass. 1587/1587 uni-store and uni-query, 3346/3346 uni-db/uni-algo/uni-query-functions. Fixes rustic-ai#177
`e16b37df8` bound a MERGE match with its properties so `ON MATCH SET n.p = n.p + x` could read them from memory instead of going to storage per row. It did so unconditionally, and the rebind immediately below it — which exists for RETURN fidelity — already ran unconditionally too. So a MERGE with no `ON MATCH SET` started paying two property reads per matched node where it had paid one, and the first result was overwritten without ever being read. That is the commonest MERGE shape, not an edge case: `MERGE (a)-[e:OWNS]->(b)` has no `ON MATCH SET` at all. Caught by `issue_225_both_spellings_of_the_same_link_cost_alike` in a full workspace run — a wall-clock ratio test over exactly that shape. It passes in isolation, which is how a change like this stays invisible: the extra read is cheap enough per row to disappear at small scale and to show up only against a timing threshold under load. The pre-SET read is now gated on `on_match` being present, which is the only thing that consumes it. 7179/7179 across the workspace. Refs rustic-ai#220
Nine query shapes ran and only "none of them errored" was asserted, which cannot tell "supported and working" from "supported and silently empty". Three of the nine are satisfied by a group variable that never bound anything — `all()` over an empty list is vacuously true, so the `WHERE` passes and rows come back for the wrong reason. No defect was hiding behind it. Measured while filing: `y` binds `[2, 4]`, and the `> 999` control already returned zero rows. So this is hygiene against a future regression rather than a repro, and the issue says so. The execution loop stays — it is the cheapest way to hold nine shapes to "still legal", and most have exact-value equivalents in the sibling tests above. What is added is the piece with no equivalent anywhere: the negative `all()` twin, whose whole point is that emptiness would produce *more* rows rather than fewer. It comes with its own control. A `> 999` assertion that returned no rows for an unrelated cause — a broken prefix, a fixture that never matched — would pass while proving nothing, so the satisfiable `> 0` twin is asserted first. Fixes rustic-ai#259
`flush_to_l1` refuses to claim a durability barrier it cannot honour, which is right and is why the condition is visible at all. But the error carried only a count, and the one observed occurrence could not be diagnosed afterwards: it did not reproduce, disk space and `TMPDIR` were both ruled out directly, and by the time anyone looked there was nothing left but the number. The cause is captured where it is already in hand — the finalizer's failure arm, before the error is consumed by `finalize_failure` — and printed in the error that stops the run. Only the most recent is kept: a failure here strands an L0, and the first one is normally the whole story. This does not explain the original occurrence, which remains unexplained. It means the next one arrives with its reason attached. `FaultBackend` grew a write-failure toggle to drive the test. It arms create/open/append rather than `write` alone, because the flush reaches Lance through whichever of those the table's state calls for — arming `write` by itself left the flush succeeding and the test asserting nothing. Verified discriminating: with the cause dropped from the message the test fails on the count-only text it used to print. Fixes rustic-ai#200
The auto-embed parity matrix had two cells left: a query-time *text* success path for multi-vector and hybrid, and call-count assertions for the sparse and multi paths. Writing the first one found that it had nothing to assert — the path was not wired. `uni.vector.query` with a string against a `List<Vector>` column rejected it outright with "Multi-vector query must be a list of vectors", even with a runtime configured. Only the *error* path had a test (`multi_autoembed_string_query_requires_runtime`), and that test passes whether or not the success path exists, which is exactly how an unwired path stays invisible. The issue anticipated this and said it would become a separate bug; it is small enough to fix here, so it is fixed here. Two gaps, not one: * **Multi-vector.** No `auto_embed_multi_text` existed beside its dense and sparse siblings. Added, honouring `query_prefix` for the same reason the dense path does — an asymmetric model returns a different vector without it, and silently worse recall is the failure mode. * **Hybrid.** A hybrid alias resolves only through `hybrid_embedder`: `runtime.embedding` and `runtime.multi_vector_embedder` are per-task resolvers and report a capability mismatch for it. So a column whose `embedding_config` names a hybrid model — the ordinary BGE-M3-style setup, where one alias fills both a `Vector` and a `List<Vector>` column — could not answer a text query on *either* head. Both now fall back to the hybrid model's single forward pass, taking the head they need, and surface the per-task resolver's own error when the alias is not hybrid. Tests: the multi and hybrid success paths (the hybrid one exercising both heads from one fixture), and batched-inference call counts for sparse and multi-vector, which the dense path had and its siblings did not. Fixes rustic-ai#122
An opt-in per-term reweighting that boosts rare terms and discounts ubiquitous ones. Off by default and deliberately so: SPLADE-style learned-sparse weights already encode term importance during training, so IDF is largely redundant there and can double-count. It earns its keep on BM25-like and BGE-M3 sparse heads, whose raw weights are closer to term-frequency than to calibrated importance. Applied **query-side**, once, before retrieval. That is what keeps the index's candidate generation and the caller's exact `sparse_dot` re-score consistent: both consume the same reweighted query, so neither can undo the other. Applying it inside the index alone is the trap the issue names, and the test `idf_reaches_the_exact_rescore` is there to catch it — it compares the reported score across the two configurations, because a score that did not move means the re-score scored against the raw query. `df(t)` is the posting-list length, read under the same `term_id IN (...)` filter the query scan already uses, so it costs one extra indexed scan over the query's own terms rather than anything proportional to the corpus. `N` is the label's row count, already available from rustic-ai#260's statistic. The form is BM25's smoothed idf, `ln(1 + (N - df + 0.5) / (df + 0.5))`, not a raw `ln(N / df)`. Raw idf goes **negative** for a term in more than half the corpus, which does not merely discount that term — it flips the sign of its contribution, so a document matching a common term would score worse than one matching nothing. It declines rather than guesses: an unknown corpus size, a missing index, or a term the index has never seen all leave the weight untouched. Settable from the schema builder (`IndexType::Sparse { idf, .. }`), from Cypher DDL (`OPTIONS{type:'sparse', idf:true}`), and from the Python bindings, so the three surfaces agree. Tests are a pair: with the modifier off the heavy ubiquitous-term document ranks first, with it on the discriminating one does. The off-case is the control — without it the positive test would prove only "the top result is `rare`", which a corpus quirk could produce on its own. Marked breaking for the new field on the public `IndexType::Sparse` variant. Fixes rustic-ai#120
`fxhash` was the only advisory in `deny.toml`'s ignore list on a crate we declare directly (five crates: uni-common, uni-algo, uni-query, uni-crdt, uni-query-functions). Unmaintained, not vulnerable, so nothing was blocked — but the exception carried a TODO naming this issue, and it can now go. Usage was purely the type aliases, so the edit is mechanical: `FxHashMap` / `FxHashSet` imports move to `rustc_hash`, and `FxBuildHasher::default()` drops the `::default()` because rustc-hash 2.x makes it a unit struct. `rustc-hash` was already in the lock transitively, so this adds no supply-chain surface. The reason this wanted its own change is iteration order: swapping a hasher changes it, and several paths here walk hash containers. The site that genuinely does is `uni-crdt`'s `orset.rs` (`.keys()` / `.values()`), which is where review effort went. Full workspace run: 7179/7179. `fxhash` remains in the tree transitively, through `biodivine-lib-bdd`, which is not ours to change. That is why removing the ignore is correct rather than optimistic: `unmaintained = "workspace"` scopes the check to workspace-direct dependencies, so with the direct dep gone the advisory no longer fires at all — `cargo deny check advisories` is green with no exception for it. The ignore entry is deleted rather than kept as a comment, and the header note that listed fxhash among crates "none of them ours" is corrected — it was ours, declared directly, which is what made it the one actionable entry in that list. Fixes rustic-ai#174
rustic-ai#242 covered the operators whose reservation was smaller than what they held. This is the other half: sixteen that never reserved at all. Measure first, as the issue asks. `examples/operator_census.rs` plans every LDBC SNB interactive-complex query against SF1 and counts physical operators. Three of the sixteen appear: OptionalFilterExec x6 (IC1 x4, IC5, IC10), GraphShortestPathExec x3 (IC1, IC13, IC14), GraphUnwindExec x3 (IC6, IC9, IC14). The other thirteen appear in none of the fourteen. Then run them, under a 1 GiB per-query ceiling. Thirteen complete. The one refusal is IC14, naming GraphTraverseExec at 4.2 GB -- an operator that already reserved, and not one of the sixteen. So no operator on this list is attributable to an over-budget query in this corpus, and nothing here claims an LDBC win. IC14 looked like the attribution for GraphShortestPathExec, being an allShortestPaths query the remediation doc records as killed by hand after 111 min at 19.2 GB and climbing; the pool refuses it upstream, long before the enumeration. Four of the fourteen return zero rows, so their parameters select nothing and they measure nothing, which is the same problem as rustic-ai#227. Disposition of all sixteen: - PowerStepExec and GraphGatherStepExec are unreachable -- no planner path constructs them -- and already classified as such by the plan-shape registry. Dead code allocates nothing. - GraphUnwindExec was already bounded by rustic-ai#241 and FixpointExec already enforces max_derived_bytes, so accounting was the whole gap for both. - GraphShortestPathExec had a real bound available: its BFS queued (Vid, Vec<Vid>) and cloned the entire partial path on every enqueue, costing O(|V| x depth) for a search whose result is one path. A parent link rebuilds the same path for O(1) per vertex. allShortestPaths itself cannot be bounded -- every path is a row the query asked for, and the count is the product of the predecessor counts along the layers -- so there, accounting is the whole remedy. - The remaining eleven reserve. collect_all_partitions was the single largest lever at thirteen call sites, none of them accounted. Charge as the structure grows, not after: try_collect resolves only once the whole input is resident, so a budget checked after it records a peak that has already happened. common::collect_accounted replaces it at every eager barrier in df_graph. The measure itself needed care, and neither of Arrow's size functions would do. get_array_memory_size reports full buffer capacity, so batches sharing buffers each report the whole parent: charging a mutation's input that way exhausted a 1 GiB pool on ten thousand rows, because the join emits one row per batch and every one-row batch claimed 3.26 MB. get_slice_memory_size narrows an array to its own window but does not push that window into nested children, which leaves a List(Utf8) child unnarrowed and the figure at 420 KB per row. The tell in both cases was that the number did not move when the input was cut by four. So BatchFootprint keys on buffer identity and charges each allocation once. That key is unsound on its own -- free an array and the allocator hands the same address back, which the unshared_batches control caught as ten independent arrays charged 36 772 bytes -- so the footprint holds an Arc per counted buffer and liveness is structural rather than asserted. Each fix carries a probe that is a required failure: an unaccounted allocation passes every ceiling, so the assertion demands a refusal naming the operator, paired with a higher ceiling asserting the answer is unchanged. All three were reverted and confirmed failing first. Two existing tests put their ceiling on the database, which applies it to the fixture's own writes as well. Both began failing inside their UNWIND ... CREATE seeding once the write path accounted honestly -- reporting a defect in a fixture rather than in the operator each was written to guard. They now put the ceiling on the query. Tests: 7184/7184 workspace; clippy -D warnings clean; fmt clean; cargo deny check advisories ok. Fixes rustic-ai#261
The reservations added for rustic-ai#242 carry the same over-count that rustic-ai#261's did. `get_array_memory_size` reports the full capacity of every buffer an array points into, so batches sliced from a common parent each report the whole parent, and adding those figures up bills the same allocation once per batch. Three sites summed that way: VidLookupJoinExec's build loop and its probe-chunk loop, and GraphTraverseMainExec's buffered input. They now charge against a BatchFootprint, which counts each allocation once. Every other `get_array_memory_size` left in the tree is a single-batch `try_resize`, and those are correct as they stand: holding a one-row slice of a large buffer really does keep that whole allocation resident, so the parent's capacity is the honest charge rather than an over-count. The defect is specific to adding them up across batches that share. Said so on BatchFootprint, so the correct sites are not "fixed" later. Two tests had their ceilings swept against the inflated figures and so passed by fitting rather than by refusing. Re-swept, not loosened: - a_vid_lookup_join_accounts_for_its_derived_index: 20 MB -> 6 MiB. The join refuses between 2 and 6 MiB on its 60 000-row fixture and the query fits from 8 MiB up. - a_vid_lookup_join_reserves_what_it_materializes: 4 MiB -> 3 MiB, and the query returns a count rather than the names. With an honest charge the join's cost and the result size land within half a megabyte of each other, so the post-hoc result-size check won the race and refused with a message naming no operator -- the confound that test's own comment warns about. Removing it beats tuning around it. The join refuses between 2 and 3 MiB; below that GraphScanExec refuses first. Both re-verified discriminating at the new ceilings: neutering only the derived-structure charges fails the first, neutering only the materialization charges fails the second. Tests: 7184/7184 workspace; clippy -D warnings clean; fmt clean. Refs rustic-ai#261
A full local CI pass found both gates red. Neither runs in pr.yml, so neither could have failed the PR that introduced them -- the same shape as the last time two ci.yml-only gates went red unnoticed. rustdoc, three broken intra-doc links: - `locy_aggregates.rs` wrote the unit interval as a bare `[0,1]` twice, which rustdoc reads as a link target. - `property_manager.rs` linked `MainEdgeDataset::find_types_by_eids_counted` with no path; the type is not in scope there. Now linked by full path, so the reference still resolves for a reader. - `vid_lookup_join`'s module doc is public and linked `DynamicVidFilter`, which is `pub(crate)`. Demoted to a code span rather than widening the type's visibility for a comment. Locy TCK in sidecar mode: `HavingInRecursivePath.feature` and `RequireInRecursion.feature` landed without their `*.schema.json` companions -- 72 features against 70 schemas -- and the sidecar lane resolves a schema per feature, so nine scenarios failed with `Missing sidecar schema` before a single step ran. The default lane derives the schema from the feature and is unaffected, which is why this was invisible outside ci.yml. Generated with the repo's own generate_feature_schemas.py rather than hand-written, so they match the shape of the other seventy. Verified: rustdoc -D warnings clean; Locy TCK 528/528 in both schema modes; openCypher TCK 3925/3925 in both; workspace 7184/7184; clippy -D warnings clean.
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
1 participant
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.
No description provided.