perf(manifest): load trigrams lazily via a path-keyed sidecar - #43
Merged
Conversation
Trigram sets are only useful to a query carrying a text predicate, but every
manifest load paid for them — a plain `WHERE service=… AND ts>…` deserialized
the whole index for nothing. They now live in a sidecar,
`{table}/_metadata/trigrams.json`, fetched only when a `MessageContains`
predicate is present.
**Keyed by file path, and that is what makes it safe.** Data files are
immutable — compaction writes new paths rather than rewriting one — so a
path's trigram set is correct forever once written. A sidecar that has fallen
behind the manifest can therefore only be *missing* entries, never hold wrong
ones, and a missing entry is `None`, which never prunes. The stale case
degrades to "scans more files than necessary", not "drops a real match", so no
atomic two-object commit is needed. Keying by index, or reusing paths, would
turn that benign staleness into silent data loss at query time.
The ordering rules follow from it:
- The sidecar is written *before* the manifest commit. It may run ahead (an
entry for a file not yet listed is never looked up) but must not lag, or a
text query loses pruning it was entitled to.
- Retired entries are pruned strictly *after* a successful commit. Pruning
first could delete an entry the still-live manifest references.
- Sidecar writes are a plain put with no CAS: entries are path-keyed and files
immutable, so concurrent writers can only add disjoint or identical values.
There is nothing to lose, and a lost race costs a missing entry, which is
safe.
`message_trigrams` is `#[serde(skip)]`, so it defaults to `None`. Forgetting to
hydrate is therefore not a correctness bug — pruning simply does not happen —
but it is a silent performance cliff, so the decision lives in one place:
`Ingestor::load_manifest_for(&preds)` fetches the sidecar only when the
predicates can use it. Both query paths in serve.rs now build predicates before
loading the manifest so they can route through it. `select_files` is untouched
and remains the single pruning choke point.
Tests:
- `load_manifest_for_fetches_trigrams_only_when_text_can_use_them` covers the
decision point directly — empty and non-text predicates stay lean, any text
predicate hydrates, and one among several is enough.
- `free_text_prunes_files_and_survives_compaction` now asserts both paths: a
lean load carries no trigrams *and* prunes nothing on text (conservative, not
wrong), while a hydrated load prunes as before. It also asserts the sidecar
holds only live paths after compaction, which caught that `retain_paths` had
been written but never wired.
- The query-crate fixture hydrates, since those tests exercise pruning.
That last one only surfaced in the `datafusion` lane: scoping a test run to one
crate hid it, exactly as rust.yml's comment warns.
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
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.
Trigram sets are only useful to a query carrying a text predicate, but every manifest load paid for them — a plain
WHERE service=… AND ts>…deserialized the whole index for nothing.They now live in a sidecar,
{table}/_metadata/trigrams.json, fetched only when aMessageContainspredicate is present.Keyed by file path — and that is the whole design
Splitting state across two objects normally raises a consistency problem: a reader could pair manifest v2 with a stale sidecar, and stale trigrams could prune away real matches. Keying by path removes it.
Data files are immutable. Compaction writes new paths rather than rewriting one, so a path's trigram set is correct forever once written. A sidecar that has fallen behind can therefore only be missing entries, never hold wrong ones — and a missing entry is
None, which never prunes.The stale case degrades to "scans more files than necessary", not "drops a real match". No atomic two-object commit needed.
The failure direction is the point. Keying by index, or reusing paths, would turn benign staleness into silent data loss at query time.
The ordering rules follow from it:
Fails safe, and hard to get wrong
message_trigramsis#[serde(skip)], defaulting toNone. Forgetting to hydrate is not a correctness bug — pruning simply doesn't happen — but it is a silent performance cliff, and silent is the part worth designing out.So the decision lives in one place:
Ingestor::load_manifest_for(&preds)fetches the sidecar only when the predicates can use it. Both query paths inserve.rsnow build predicates before loading the manifest so they can route through it.select_filesis untouched and remains the single pruning choke point, so the quote-equals-scan invariant is unchanged.Tests
load_manifest_for_fetches_trigrams_only_when_text_can_use_them— covers the decision point directly: empty and non-text predicates stay lean, any text predicate hydrates, one among several is enough. It had no direct coverage before, which is a gap in a function whose only job is making a choice.free_text_prunes_files_and_survives_compaction— now asserts both paths: a lean load carries no trigrams and prunes nothing on text (conservative, not wrong), while a hydrated load prunes as before. Also asserts the sidecar holds only live paths after compaction — which caught thatretain_pathshad been written but never wired.That last one only surfaced in the
datafusionlane. Scoping a test run to one crate hid it, exactly asrust.yml's comment warns: "the default build has no query engine, so code behinddatafusion/serveis invisible to a default-features run."Verification
scripts/verify.sh --all rust→ OK (three test lanes, both clippy lanes, check-apply, fmt).Closes #6.