Add prefix and substring string predicates with index acceleration#358
Add prefix and substring string predicates with index acceleration#358ragnorc wants to merge 3 commits into
Conversation
…nguage starts_with is a new filter keyword; contains is overloaded on the left operand's type (list = membership as before, scalar String = exact substring — previously a type error, so no existing query changes meaning). Both are exact and case-sensitive, with NULL never a match. The overload is resolved at lowering time into a distinct IR op (StringContains), so execution dispatches on the IR alone and never re-derives operand types. Predicates lower to the DataFusion starts_with / contains function exprs, whose names Lance's scalar-index expression parser maps to BTREE LikePrefix (exact) and NGRAM StringContains (probe + recheck) probes when a covering index exists; without one they execute as correct filtered scans. Standalone string-match filters on a scanned variable are hoisted into the NodeScan's filter_expr so they can reach a covering index; new instrumentation probes (pushed_filter_exprs / in_memory_filters) pin the hoist structurally, since results alone cannot distinguish it from a silent full-scan fallback. Mutation predicates keep rejecting the new operators at parse time, with defensive arms behind them. The datafusion string_expressions feature supplies the two function builders.
Three new surface guards cover the substrate behaviors the string predicates and the upcoming index work rely on: - starts_with on a BTREE'd column plans a scalar-index probe (LikePrefix) and treats _ / % in the needle as literal bytes, never LIKE wildcards. - contains on an NGRAM'd column plans a scalar-index probe (StringContains) whose recheck keeps results exact, including needles below the trigram width. - Lance's default index name is shared per column and replace removes by name, so an unnamed second-index build must replace or refuse — never coexist — while an explicitly-named second index of a different type coexists. Any second index on a column therefore requires an explicit distinct name.
A free-text (non-enum) String @index/@key column previously got only an FTS inverted index, which Lance never consults for equality or prefix filters — both fell back to full scans (the documented gap in docs/user/search/indexes.md). The index planner now additionally plans a BTREE on such columns, so =, range, and starts_with are index-accelerated; existing graphs converge on their next ensure_indices/optimize run. The companion BTREE carries an explicit name ({column}_btree_idx), threaded through IndexBuildSpec::BTree: Lance's default index name is shared per column and replace removes by name, so an unnamed second build would silently replace the FTS index (pinned by the index-naming surface guard). Both indexes land in the same staged CreateIndex transaction, so the table still advances exactly once and the recovery protocol is unchanged. Index-count expectations in the search and branching suites move to the new totals.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 549c04b. Configure here.
| CompOp::StringContains | ||
| } else { | ||
| CompOp::Contains | ||
| } |
There was a problem hiding this comment.
Reversed contains uses wrong semantics
Medium Severity
The String overload of contains is keyed off any String-typed left operand (literal, query param, or property), but execution always applies left.contains(right). Queries such as $q contains $p.name or "needle" contains $p.name now typecheck and lower to StringContains, yet they test whether the constant or param contains each row’s property—not substring search on the column as documented.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 549c04b. Configure here.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .push(filter.clone()); | ||
| hoisted_indices.insert(i); | ||
| } | ||
| } else if is_string_match_filter(filter) | ||
| && ir_filter_to_expr(filter, params, None).is_some() | ||
| { | ||
| if let IRExpr::PropAccess { variable, .. } = &filter.left { | ||
| if scan_vars.contains(variable.as_str()) { | ||
| hoisted_search_filters | ||
| .entry(variable.clone()) | ||
| .or_default() | ||
| .push(filter.clone()); | ||
| hoisted_indices.insert(i); | ||
| } | ||
| } |
There was a problem hiding this comment.
When a string predicate compares the scanned variable to another variable, such as a.name starts_with b.prefix, this branch hoists it into a's NodeScan and skips the original filter. The pushed expression drops the b variable and resolves prefix inside a's table, so callers can get incorrect rows or a scan-planning error instead of evaluating the predicate after both variables are bound.
Context Used: AGENTS.md (source)
| .get(v) | ||
| .is_some_and(|t| !t.list && matches!(t.scalar, ScalarType::String)), | ||
| _ => false, |
There was a problem hiding this comment.
contains is accepted by typecheck for any scalar String left expression, but lowering only recognizes properties, literals, and params as the String overload. If a String alias expression reaches this fallback, it stays as CompOp::Contains, so execution uses list-membership lowering on a scalar String and the query errors instead of doing a substring match.
Context Used: AGENTS.md (source)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 549c04b003
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let IRExpr::PropAccess { variable, .. } = &filter.left { | ||
| if scan_vars.contains(variable.as_str()) { | ||
| hoisted_search_filters | ||
| .entry(variable.clone()) | ||
| .or_default() | ||
| .push(filter.clone()); | ||
| hoisted_indices.insert(i); |
There was a problem hiding this comment.
When a starts_with/String contains filter compares one scanned variable with another (for example, match { $a: Metric $b: Metric $b.label starts_with $a.label }), this branch hoists it into $b's NodeScan and removes the original Filter. The scan-level lowering drops variable names and uses bare column identifiers, so the predicate is evaluated against $b's table only (or errors if the other property is absent) instead of after the cross join, producing incorrect results for cross-variable filters. Only hoist predicates whose property operands all belong to the scanned variable or are literals/params.
Useful? React with 👍 / 👎.
| Expr::PropAccess { variable, property } => type_ctx | ||
| .bindings | ||
| .get(variable) | ||
| .and_then(|bv| catalog.node_types.get(&bv.type_name)) | ||
| .and_then(|nt| nt.properties.get(property)) | ||
| .is_some_and(|p| !p.list && matches!(p.scalar, ScalarType::String)), |
There was a problem hiding this comment.
For contains filters inside not { ... } that reference a variable introduced only inside the negation, this lookup uses the outer TypeContext, which intentionally does not retain negation-local bindings from typecheck. A query such as not { $m tagged $t $t.tname contains "sal" } typechecks as String substring matching, but lowering leaves it as list Contains; execution then tries to evaluate a String column as a list and fails at runtime instead of applying the substring predicate. The overload resolution needs the current inner binding context when lowering nested clauses.
Useful? React with 👍 / 👎.
| work.push_spec(crate::storage_layer::IndexBuildSpec::BTree { | ||
| column: prop_name.clone(), | ||
| name: Some(format!("{prop_name}_btree_idx")), | ||
| }); |
There was a problem hiding this comment.
The generated {prop_name}_btree_idx can collide with Lance's default {column}_idx for another indexed column, e.g. a schema with text: String @index and text_btree: I32 @index gives both BTREEs the name text_btree_idx. In one pass stage_create_indices rejects duplicate new names; across passes, Lance removes/replaces indexes by name, so the two indexes can keep deleting each other and ensure_indices never converges. Use a reserved/collision-checked naming scheme or include disambiguation that cannot be produced by a user column's default index name.
Useful? React with 👍 / 👎.
1 participant
What & why
Adds exact string predicates to the query language —
starts_withand a String overload ofcontains(substring) — so prefix/autocomplete and substring lookups are expressible in.gq, and makes them index-accelerated: predicates lower to structured DataFusion exprs that Lance answers from a BTREE (LikePrefix, exact) or NGRAM (StringContains, probe + recheck) when one covers the column, and free-text@indexString columns now get an explicitly-named companion BTREE alongside their FTS index so prefix and equality filters stop full-scanning.Backing issue / RFC
Maintainer change — internal tracking applies.
Checklist
Notes for reviewers
containsoverload is backward-compatible by construction: a scalar-String left operand was a type error before, so no existing query changes meaning; the overload is resolved at lowering time into a distinct IR op (semantics stay first-class in typed IR, never re-derived at execution).filter_exprso they can reach a covering index; the hoist is pinned structurally via new instrumentation probes (results alone cannot distinguish it from a silent full-scan fallback).replaceremoves by name, so an unnamed second index would silently replace the FTS index — the companion BTREE therefore carries an explicit name, and a new surface guard pins the collision behavior.ends_withand glob/regex are deliberately not exposed (no acceleration path / pattern-dependent cost).@indexkind argument to accelerate substringcontains.Note
Medium Risk
Changes query semantics (new overload), index materialization on
ensure_indices(extra BTREEs on existing FTS columns), and scan pushdown paths; Stringcontainsoverload is backward-compatible because it was previously a type error.Overview
Introduces
starts_withand a String overload ofcontains(exact substring) in match filters, with parser/grammar support and mutation grammar still rejecting these ops.Compiler: Typecheck accepts list
contains(membership) or scalar Stringcontains(substring); lowering maps Stringcontainsto a distinctStringContainsIR op so runtime never re-derives types. NewCompOp::StartsWith/StringContainsvariants.Execution: In-memory filters implement exact, case-sensitive string matching (NULL → no match). Standalone predicates on a NodeScan variable are hoisted into scan
filter_expralongside FTS search filters; pushdown uses DataFusionstarts_with/containsso Lance can use BTREE (LikePrefix) or NGRAM (StringContains) when present. Test probes count pushed vs in-memory filters.Indexes: Free-text String
@indexcolumns now plan FTS plus an explicitly named companion BTREE ({column}_btree_idx) so equality andstarts_withare not full scans;IndexBuildSpec::BTreegains optionalnameto avoid Lancereplaceclobbering the FTS index on the default name.Other: Enables DataFusion
string_expressions; user docs distinguish exact predicates from token-based search; Lance surface guards pin BTREE/NGRAM routing and dual-index naming.Reviewed by Cursor Bugbot for commit 549c04b. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR adds exact string predicates with Lance index acceleration. The main changes are:
.gqstarts_withsyntax and scalar-Stringcontainsoverload.@indexcolumns.Confidence Score: 4/5
The scan hoist for string predicates needs a fix before merging.
containsexpression.crates/omnigraph/src/exec/query.rs and crates/omnigraph-compiler/src/ir/lower.rs
Important Files Changed
starts_withand the scalarcontainsoverload.containstoStringContains, with a narrow fallback gap for String alias-like expressions.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Filter: a.name starts_with b.prefix] --> B[Hoist pre-pass checks left variable a] B --> C[Filter attached to NodeScan a] C --> D[Original Filter skipped] D --> E[PropAccess b.prefix becomes column prefix] E --> F[Scan reads prefix from a table] F --> G[Incorrect rows or planning error]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[Filter: a.name starts_with b.prefix] --> B[Hoist pre-pass checks left variable a] B --> C[Filter attached to NodeScan a] C --> D[Original Filter skipped] D --> E[PropAccess b.prefix becomes column prefix] E --> F[Scan reads prefix from a table] F --> G[Incorrect rows or planning error]Reviews (1): Last reviewed commit: "Build a companion BTREE alongside FTS on..." | Re-trigger Greptile
Context used: