Repository navigation
perf(fix): restore per-pattern entity priority in selective_fact_fetch - #283
Merged
Merged
Conversation
#282) `bb43871` introduced `selective_fact_fetch` but collected all bound entities and all bound attributes across all patterns into two separate sets, then unioned both lookups. For a query like `[:e0 :val ?v]` this caused a broad attribute scan (all `:val` facts) on top of the entity lookup, degrading what should be a µs point-lookup into ms-range scans at scale (2.3–4.9× slower in the concurrent_file/readers_plus_writer benchmark family, confirmed by A/B local Criterion run). Fix: per-pattern selection. A pattern with a bound entity uses the entity index and skips its attribute; a pattern without a bound entity falls back to an attribute lookup if one is available; a pattern with neither causes the whole fetch to fall back to a full scan, which is required for correctness since the fact universe for that pattern cannot be restricted. Regression test added: `test_selective_fetch_prefers_bound_entity_over_attribute_scan` verifies that a bound-entity + bound-attribute pattern fetches only the entity's facts (2), not the entity's facts ∪ all attribute facts (22). Closes #282 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Summary
bb43871(perf: predicate push-down for Expr clauses + mixed rule optimization (#207, #206) #249) introducedselective_fact_fetchbut incorrectly collected bound entities and bound attributes into two separate global sets, then issued lookups for both — even when a pattern already had a bound entity.[:e0 :val ?v]this caused a full attribute scan (get_facts_by_attribute(":val")) on top of the entity lookup, turning a µs operation into an ms scan at scale.concurrent_file/readers_plus_writervariants; this matches the nightly Bencher signal starting May 16 (see Benchmark regression - 2026-05-21 #282).Fix
Per-pattern selectivity: for each pattern, if a bound entity is present, use the entity index and skip adding the attribute. If no bound entity but a bound attribute exists, use the attribute index. If a pattern has neither, fall back to a full scan (required for correctness — the fact universe cannot be restricted for that pattern).
Regression Test
test_selective_fetch_prefers_bound_entity_over_attribute_scan— transacts 21 entities (e0..e20), queries[:e0 :val ?v], asserts the candidate set is 2 facts (e0's facts only, not the 22 from entity ∪ attribute union). Test fails against the buggy code withleft: 22, right: 2.Test Plan
cargo test test_selective_fetch— new regression test passescargo test --test complex_queries_test— 11/11 passcargo test— full suite passes (pre-push hook verified)Closes #282
🤖 Generated with Claude Code