Skip to content

Fix reactive filter-field index trigger and partial index merge correctness - #1

Open
ocean wants to merge 6 commits into
nyrkio:mainfrom
ocean:feature/filter-field-indexes
Open

ocean wants to merge 6 commits into
nyrkio:mainfrom
ocean:feature/filter-field-indexes

Conversation

@ocean

@ocean ocean commented Apr 10, 2026

Copy link
Copy Markdown

Hi Henrik! I'm not sure if you're wanting any contributions or taking PRs for this project yet, but I stumbled across a link to it and was fascinated, so I've forked and dug into it, and found a couple of bugs and gaps (with Claude's help of course) that I thought I would fix.

I'm not any kind of database or Rust expert by any means, but someone who is constantly trying to learn about new things, loves learning new languages and patterns, and comes at projects with 20 years of general technical experience across a variety of languages and codebases, and loves the spirit of open source.

Two bugs were found in the reactive secondary index machinery, both of which would prevent filter-field indexes from working correctly in practice. Also a small re-export fix and a PLAN.md update.

Bug 1 - Filter index never built for selective queries

The trigger for building a reactive filter-field index compared results.len() (documents returned) against sort_spill_threshold. For a highly selective filter - say with a 1% match rate - results.len() is tiny even when the scan examined every document in the collection. The index would not built for these small queries which would benefit from it.

Fix: use docs_scanned instead. An index is now triggered whenever the scan cost exceeds the threshold, regardless of how many documents matched.

Bug 2 - Partial index merge produced false full-range index

When maybe_compact_indexes() found multiple partial indexes for the same sort field, it combined their entries and wrote them back with range=None, falsely claiming full-range coverage. A subsequent no-filter sort scan would use this index and miss every document not covered by any of the partial filter ranges.

Two problems in one:

  1. The merge used build_partial(..., None, &mut combined_entries, ...) - partial data, full-range claim.
  2. No IndexMetadata was pushed to pending_index_metadata after the merge, so the resulting index was invisible to Database and lost on restart.

Fix: replace the merge with SecondaryIndex::build() - a full rebuild from primary SSTables - which guarantees complete coverage. Persist the metadata afterwards.

Minor: Database not exported from the top-level ingodb crate

Database was defined in ingodb-lsm but missing from the re-exports in ingodb/src/lib.rs, making it unreachable via the public API.

PLAN.md

Phase 5b (MVCC Snapshot Reads) was marked as "Remaining" but is fully implemented - MvccKeyExtractor, get_at()/scan_at(), active_snapshots GC, MVCC-aware compaction dedup etc. Moved to "Completed". Also added compact_now(), a public synchronous flush + compact entry point for use in tests and benchmarks.

Tests

  • test_filter_index_built_for_selective_queries - 500 docs, 5 matching (1% selectivity), threshold=100. Verifies no index after scan 1, index present after scan 2, correct results on scan 3.
  • test_filter_index_not_built_below_scan_threshold - 100 docs, threshold=1000. Verifies no index when the scan itself is cheap.
  • test_filter_index_survives_restart - triggers index creation, drops and reopens Database, asserts index reloads from system collection with correct results.
  • test_partial_index_merge_produces_full_coverage - creates partial indexes for two filter ranges (alpha, beta), compacts, asserts a no-filter sort scan returns all 30 documents including the uncovered gamma range.
  • test_query_stats_recorded updated: expected total_scanned corrected from 100 to 52, reflecting that scans 3–5 now use the index (4 docs scanned each instead of 20).

ocean added 6 commits April 10, 2026 15:11
A highly selective filter (e.g. 1% match rate) scans all N documents but
returns very few. The old trigger compared results.len() against the spill
threshold, so indexes were never built for exactly the queries that need
them most.

The trigger now uses docs_scanned — the actual cost of the full scan —
so an index is built whenever the scan work exceeds the threshold,
regardless of how many documents match the filter.
Two bugs in maybe_compact_indexes() when multiple partial indexes exist
for the same sort field:

1. The old code combined entries from the partial indexes and wrote them
   back with range=None, falsely claiming full-range coverage. A
   subsequent no-filter sort scan would miss every document not covered
   by any of the partial filter ranges.

   Fix: use SecondaryIndex::build() to rebuild from primary SSTables,
   which guarantees all documents are included.

2. After a successful merge, no IndexMetadata was pushed to
   pending_index_metadata, so the merged index was invisible to Database
   and lost on restart.

   Fix: push metadata with is_full_range=true after each successful merge.

Also adds compact_now() — a public synchronous flush+compact entry point
useful in tests and benchmarks.
Database was defined in ingodb-lsm but not re-exported from the ingodb
facade crate, making it inaccessible to callers using the public API.
Three new integration tests covering the filter-index behaviour:

- test_filter_index_built_for_selective_queries: 500 docs, 5 matching
  (1% selectivity). Verifies no index after scan 1, index present after
  scan 2 (docs_scanned=500 > threshold=100), correct results via index
  on scan 3.

- test_filter_index_not_built_below_scan_threshold: 100 docs, threshold
  1000. Verifies no index is built when the scan itself is cheap.

- test_filter_index_survives_restart: triggers index creation, drops
  the Database, reopens it, and asserts the index is reloaded from the
  system collection with correct results.

Also updates test_query_stats_recorded: the old assertion expected
total_scanned=100 (5 full scans × 20 docs). With the fixed trigger a
filter index is built after scan 2, so scans 3-5 use it
(docs_scanned=4 each). Total is now 20+20+4+4+4 = 52, which
confirms the index is actually reducing scan work.
Verifies the fix to maybe_compact_indexes(): when two partial indexes
(alpha range, beta range) are merged at compaction, a subsequent
no-filter sort scan returns all 30 documents — not just the 20 that
belonged to the indexed filter ranges.

Also exercises compact_now() as the synchronous compaction entry point.
- Mark Phase 5b (MVCC Snapshot Reads) as completed — MvccKeyExtractor,
  get_at()/scan_at(), active_snapshots GC are all implemented.
- Add note to Phase 5a: partial index merge now uses full rebuild from
  primary SSTables to ensure complete coverage.
@henrikingo

Copy link
Copy Markdown
Contributor

Hi! Only realized now there's PR here! How cool. I'll have a look tomorrow. (i need to blog about the concept too... it's overdue)

@ocean

ocean commented Jun 29, 2026

Copy link
Copy Markdown
Author

Hi Henrik! No problems, my changes here are out of date now anyway, I'll revise this sometime and fix it up.

I look forward to reading your thoughts on the whole concept too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants