Add prefix and substring string predicates with index acceleration#358
Add prefix and substring string predicates with index acceleration#358ragnorc wants to merge 10 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.
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 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). Red-on-fix; gates the planned opt-in NGRAM@indexkind follow-up.starts_with→ BTREELikePrefixrouting (literal_/%treatment) and the index default-naming collision behavior.