Push standalone filter clauses down to the Lance scanner - #450
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
724
to
789
| search_mode: &'a SearchMode, | ||
| ) -> std::pin::Pin<Box<dyn std::future::Future<Output = Result<()>> + Send + 'a>> { | ||
| Box::pin(async move { | ||
| // Pre-pass: collect search filters that need to be hoisted to NodeScan | ||
| // Pre-pass: hoist filters onto the op that introduces their binding. | ||
| // Search filters always move to the binding's NodeScan (applied via | ||
| // `scanner.full_text_search`). Scalar filters referencing exactly one | ||
| // binding move to that binding's NodeScan (`filter_expr`, which also | ||
| // arms `prefilter(true)` so a `nearest`/`bm25` on the same scanner is | ||
| // filtered BEFORE top-k instead of starved after it) or into the | ||
| // introducing Expand's `dst_filters` (applied during hydration). | ||
| // Multi-binding filters (e.g. the cycle-closing `temp.id = dst.id`) | ||
| // and filters on a variable not introduced here (an outer binding | ||
| // inside an anti-join pipeline) keep their end-of-pipeline placement. | ||
| let mut scan_vars: HashSet<&str> = HashSet::new(); | ||
| let mut expand_dst_vars: HashSet<&str> = HashSet::new(); | ||
| for op in pipeline { | ||
| match op { | ||
| IROp::NodeScan { variable, .. } => { | ||
| scan_vars.insert(variable.as_str()); | ||
| } | ||
| IROp::Expand { dst_var, .. } => { | ||
| expand_dst_vars.insert(dst_var.as_str()); | ||
| } | ||
| IROp::Filter(_) | IROp::AntiJoin { .. } => {} | ||
| } | ||
| } | ||
|
|
||
| let mut hoisted_search_filters: HashMap<String, Vec<IRFilter>> = HashMap::new(); | ||
| let mut hoisted_scan_filters: HashMap<String, Vec<IRFilter>> = HashMap::new(); | ||
| let mut hoisted_dst_filters: HashMap<String, Vec<IRFilter>> = HashMap::new(); | ||
| let mut hoisted_indices: HashSet<usize> = HashSet::new(); | ||
| for (i, op) in pipeline.iter().enumerate() { | ||
| if let IROp::Filter(filter) = op { | ||
| if is_search_filter(filter) { | ||
| if let Some(var) = search_filter_variable(filter) { | ||
| hoisted_search_filters | ||
| .entry(var.to_string()) | ||
| .or_default() | ||
| .push(filter.clone()); | ||
| hoisted_indices.insert(i); | ||
| } | ||
| let IROp::Filter(filter) = op else { continue }; | ||
| if is_search_filter(filter) { | ||
| if let Some(var) = search_filter_variable(filter) { | ||
| hoisted_search_filters | ||
| .entry(var.to_string()) | ||
| .or_default() | ||
| .push(filter.clone()); | ||
| hoisted_indices.insert(i); | ||
| } | ||
| continue; | ||
| } | ||
| let mut vars = filter_variables(filter).into_iter(); | ||
| let (Some(var), None) = (vars.next(), vars.next()) else { | ||
| continue; | ||
| }; | ||
| // Only pushable filters may leave their in-memory position: | ||
| // `execute_node_scan` silently ignores filters `ir_filter_to_expr` | ||
| // cannot lower (no post-scan fallback there). The schema arg only | ||
| // affects a literal's type, never Some-vs-None, so `None` here | ||
| // gives the same verdict as the scan site. | ||
| if ir_filter_to_expr(filter, params, None).is_none() { | ||
| continue; | ||
| } | ||
| let target = if scan_vars.contains(var.as_str()) { | ||
| &mut hoisted_scan_filters | ||
| } else if expand_dst_vars.contains(var.as_str()) { | ||
| &mut hoisted_dst_filters | ||
| } else { | ||
| continue; | ||
| }; | ||
| target.entry(var).or_default().push(filter.clone()); | ||
| hoisted_indices.insert(i); | ||
| } | ||
|
|
There was a problem hiding this comment.
This changes the user-visible execution semantics of standalone scalar filters, including pre-top-k filtering for search, but only updates the developer execution guide. Update the relevant docs/user query documentation so users have an accurate account of standalone equality and range-filter behavior.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
aaltshuler
force-pushed
the
filter-clause-pushdown
branch
from
August 6, 2026 20:50
d6e1b1f to
a8279ad
Compare
This was referenced Aug 6, 2026
Merged
azimafroozeh
pushed a commit
to azimafroozeh/omnigraph
that referenced
this pull request
Aug 8, 2026
Adds the missing user-visible changes to the v0.9.0 release notes: edge property bindings (ModernRelay#447), standalone filter pushdown (ModernRelay#450), the keyed per-transaction write bound with the Overwrite exemption (ModernRelay#454), strict cluster policy-bundle validation (ModernRelay#444), and developer-facing lines for RFC-030/031/032 and the rustfmt baseline.
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
2 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.
The two spellings of the same
matchpredicate executed differently: inline props ($d: Doc { status: "open" }) pushed down to the Lance scanner, while a standalone filter clause ($d.status = "open", the only form that can express ranges) ran in memory after the scan. So a filterednearest()searched top-k of the whole table and the late filter starved the result set (fewer than k rows, possibly zero, exit 0), and without a search the clause form materialized every row of the table, all columns included, before dropping the non-matching ones.The fix generalizes the existing FTS-filter hoist in the executor: a pre-pass moves each filter that references exactly one binding and lowers to a structured scanner expression onto the op introducing that binding, the node scan (arming
prefilter(true)) or the expand's destination filters. Multi-binding filters, non-pushable shapes, and filters on outer bindings insidenot { }keep their in-memory position, so the fallback is exactly the prior behavior.Benchmark
Release builds, before = main. Docs with a ~216-char text field and a Vector(256) column, 1% matching
status = "open", query returns 10 slugs via the clause form. Peak RSS via /usr/bin/time over 5 runs; inline = the same predicate written inline, the target the clause form should match.2.6 GB held to return 10 rows becomes 156 MiB, identical to inline: the scan now materializes only the 1% that matches, where the old penalty grew linearly with table size.
Local verification
filtered_nearest_clause_spelling_prefilters_like_inlinein the search suite, red on unfixed code (0 rows instead of 3), covers equality and a range predicate; blob coverage extended to the clause formcargo test -p omnigraph-engine: search, traversal, traversal_indexed, literal_filters, ordering, aggregation, proptest_equivalence, end_to_end all greencargo test --workspace --locked(failpoints features): no failures attributable to this change; the 7 failing targets reproduce identically on unmodified mainGreptile Summary
The PR pushes eligible single-binding standalone filters into the scan or destination-expansion operation so filtering occurs before search top-k selection and avoids unnecessary materialization.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD F[Standalone filter] --> V{References exactly one binding?} V -- No --> M[Keep in-memory position] V -- Yes --> L{Lowerable to scanner expression?} L -- No --> M L -- Yes --> I{Binding introduced by} I -- NodeScan --> S[Attach scanner filter and enable prefilter] I -- Expand destination --> E[Attach destination filter] S --> Q[Search top-k over matching rows] E --> H[Filter during destination hydration]Reviews (3): Last reviewed commit: "review" | Re-trigger Greptile
Context used: