Add prefix and substring string predicates with index acceleration#358
Add prefix and substring string predicates with index acceleration#358ragnorc wants to merge 11 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.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 👍 / 👎.
There was a problem hiding this comment.
Real — fixed in c049642 (same fix as the sibling finding): the hoist requires a literal/param needle, so cross-variable forms evaluate in memory after the join. Pinned by literal_filters::cross_variable_string_predicate_is_not_hoisted.
| 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 👍 / 👎.
There was a problem hiding this comment.
Real — fixed in c049642. Lowering now resolves the contains overload from clause-local bindings (declarations + traversal endpoint types) before the outer TypeContext, matching how the traversal-direction fallback already handles the discarded negation-inner context. Pinned by literal_filters::string_contains_resolves_inside_negation (previously failed at runtime with 'contains requires a list property on the left').
| 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 👍 / 👎.
There was a problem hiding this comment.
Real — fixed in c049642. The companion is now named {column}_btree: every Lance default index name ends in _idx, so the two suffix classes cannot intersect for any column name. Pinned by scalar_indexes::companion_btree_name_cannot_collide_with_default_index_names (previously ensure_indices failed permanently on a schema with a text + text_btree column pair).
…lisions
Three regression tests, each failing with its bug's exact symptom:
- A cross-variable starts_with ($b.label starts_with $a.label) is hoisted
into $b's scan, where lowering drops variable qualifiers, degenerating
to a self-comparison: 12 cross-join pairs instead of the 3 real matches.
- A String contains on a variable introduced only inside not { } is left
as list-membership (negation inners are typechecked into a discarded
context clone), failing at runtime with 'contains requires a list
property on the left' instead of substring matching.
- A column literally named {other}_btree makes the companion BTREE name
{other}_btree_idx collide with that column's default index name:
stage_create_indices rejects the duplicate and ensure_indices fails
permanently for a legal schema.
Three review findings, each turning its pinned regression green:
- The scan hoist now requires a literal or param needle. Scan-level
lowering drops variable qualifiers, so a hoisted cross-variable
predicate degenerated to comparing a column with itself; cross-variable
forms stay in the in-memory arm, which evaluates on the joined wide
batch.
- The contains overload resolves variable types from the clause-local
bindings (declarations plus traversal endpoint types) before the outer
TypeContext, so variables introduced only inside not { } resolve —
negation inners are typechecked into a discarded context clone, the
same asymmetry the traversal-direction fallback documents.
- The companion BTREE is named {column}_btree instead of
{column}_btree_idx: every Lance default index name ends in _idx, so
the suffix classes can no longer intersect regardless of column names.
Also documents positional operand semantics for the string predicates
(X contains Y tests that X contains Y, either side may be a property;
acceleration applies to the property-on-the-left form).
X contains Y tests that X contains Y whichever side each operand is on (the reversed param-contains-property form), and a same-variable two-property predicate evaluates row-wise; neither form is hoisted, so both exercise the in-memory arm.
Pure-Lance repro, no engine code: a dataset's BTREE index reads work through a first-generation branch (the clone's base-path redirect resolves the index files in the root tree) but hard-error with Not found: tree/<parent>/_indices/... through a branch created FROM that branch — the second clone records its redirect against the immediate source tree instead of composing the source's own redirect. Observed identically for FTS (tokens.lance) and BTREE (page_lookup.lance) at the engine level on the fast-forward merge-into-non-main topology. The guard asserts the bug (the former blob-compaction guard pattern) and turns red when a Lance bump fixes it; its panic message carries the re-landing checklist for the deferred free-text companion BTREE. Also brings the Array trait into scope for the guard helpers.
The companion BTREE dispatch made branch_merge_into_non_main_target_works red: its @key equality lookup started probing the new index through a branch-of-a-branch fork, where Lance's second-generation shallow clones cannot read parent index files at all (Not found on tree/<parent>/_indices/...). Validation showed the bug is upstream and pre-existing — a pure-Lance repro with no engine code fails identically, and FTS search plus enum-BTREE equality on that topology are broken on main today — so shipping the companion would have widened a latent landmine to every @key equality lookup on merged branches. The dispatch and its expectations are deferred (preserved on the dual-btree-companion branch); the staged same-column machinery, explicit index naming, and their tests stay, since they are independently correct. The re-landing trigger is the surface guard pinning the bug: it turns red when a Lance bump fixes second-generation clone reads.
Two realities the new pins surfaced on their first execution: - With an NGRAM index, a contains needle below the trigram width silently returns ZERO rows: the index's at_least(empty) lower bound (recheck everything) is treated as authoritative by the scan plan. The guard now pins the buggy behavior and turns red when Lance fixes it — the NGRAM @index kind must not ship String contains that drops rows on short needles. - A bare variable as a string predicate's left operand parses as a traversal over an edge named after the keyword (clause precedence tries traversals before filters), so the reversed form is written with a literal left operand and the caveat is documented.
The two upstream bugs the string-predicate work surfaced are now filed: lance-format/lance#7840 (second-generation shallow clones cannot read parent index files) and lance-format/lance#7841 (NGRAM contains silently drops rows for sub-trigram needles). Point the surface guards, the companion-BTREE deferral comment, and the indexes doc at them so the red-on-fix triggers name their tracking issues.
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. Predicates lower to structured DataFusion exprs that Lance answers from a covering scalar index (BTREELikePrefixfor prefixes — exact; NGRAMStringContainsfor substrings — probe + recheck) and execute as correct filtered scans otherwise. Both are exact, case-sensitive, positional, and NULL-safe.Backing issue / RFC
Maintainer change — internal tracking applies.
Checklist
Notes for reviewers
Language semantics
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), with clause-local binding resolution so it works for variables introduced insidenot { }.filter_exprso they can reach a covering index; the hoist is pinned structurally via instrumentation probes (results alone cannot distinguish it from a silent full-scan fallback). Cross-variable predicates deliberately stay in the in-memory arm (review finding, regression-pinned).ends_withand glob/regex are deliberately not exposed (no acceleration path / pattern-dependent cost).Deferred: the free-text companion BTREE
A companion BTREE on free-text
@indexStrings (equality +starts_withacceleration, closing the long-documented equality-scan gap) was implemented but is deferred: it exposed a pre-existing upstream Lance bug (lance-format/lance#7840) where second-generation shallow clones (a branch of a branch, e.g. after a fast-forward merge into a non-main target) cannot read parent index files and hard-error — for every index kind, FTS included. Validated with a pure-Lance repro (no engine code) pinned aslance_surface_guards::second_generation_branch_index_reads_fail_upstream; the guard turns red when a Lance bump fixes it and its message carries the re-landing checklist. The full implementation is preserved on thedual-btree-companionbranch. The staged same-column index machinery, explicit index naming (Lance replaces indexes by name, so a second index per column must be explicitly named), and their tests land here since they are independently correct.Upstream surface pins added
second_generation_branch_index_reads_fail_upstream— the clone-of-clone index-read failure above.containsneedles shorter than the trigram width silently return zero rows (the index's recheck-everything lower bound is treated as authoritative) — filed as NGram-indexedcontainssilently returns zero rows for a needle shorter than the trigram width lance-format/lance#7841. Red-on-fix; gates the planned opt-in NGRAM@indexkind follow-up.starts_with→ BTREELikePrefixrouting (literal_/%treatment) and the index default-naming collision behavior.Note
Medium Risk
Touches query language semantics, filter pushdown/hoisting, and Lance index interaction; behavior is heavily tested but wrong hoist or overload resolution could change results or performance on string filters.
Overview
Adds
starts_withand a String overload ofcontains(exact substring) to.gqmatch filters, with compiler resolution into distinct IR ops (StartsWith,StringContains) so execution never re-derives operand types. Listcontainsstays membership; scalar Stringcontainsis new (previously a type error, so no semantic change for existing queries).Compiler/runtime: Typecheck accepts both overloads; lowering uses clause-local types so
not { }and negation inners resolve correctly. Execution evaluates string predicates in-memory and hoists standalone literal/param needles on scanned variables into NodeScanfilter_expras DataFusionstarts_with/containsfor Lance BTREE (prefix) and NGRAM (substring) probes; cross-variable predicates stay in-memory. Instrumentation counters pin hoist vs in-memory behavior in tests.Index plumbing (companion BTREE deferred):
IndexBuildSpec::BTreegains optionalnameso a second index on one column does not replace the first under Lance’s default naming; staged index tests cover FTS + named BTREE on the same column. Automatic companion BTREE on free-text@indexStrings is not enabled yet (upstream clone-of-clone index read bug). Workspace enables DataFusionstring_expressions.Tests/docs: Integration tests for pushdown, negation, composition with search, and reversal; Lance surface guards for prefix/substring index routing and index naming; user docs for string predicates vs token search.
Reviewed by Cursor Bugbot for commit 0713fe3. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR adds exact string predicates to the query language. The main changes are:
starts_withparsing, typechecking, lowering, and execution.containslowering as substring matching.Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
containsto the dedicated substring IR operation.Reviews (5): Last reviewed commit: "Link the deferred-companion guards and d..." | Re-trigger Greptile
Context used: