fix(ngram): recheck contains() with sub-trigram needle instead of dropping rows#7845
Conversation
📝 WalkthroughWalkthroughNGram ChangesNGram contains handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance-index/src/scalar/ngram.rs-117-117 (1)
117-117: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the trigram-width threshold.
Line 117 exposes a non-obvious threshold across modules without documenting its unit or rationale. Add a doc comment describing the NGram token width and why it is three.
As per coding guidelines, “Add doc comments to magic constants, thresholds, and non-obvious transformation functions, explaining what the value represents and why it was chosen.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-index/src/scalar/ngram.rs` at line 117, Document the NGRAM_N constant with a doc comment stating that it defines the NGram token width in characters and explaining why the width is fixed at three (trigrams). Keep the change scoped to the constant’s documentation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-index/src/scalar/expression.rs`:
- Around line 906-911: Update the NGram short-query guard in the contains
expression handling to compare the pattern’s Unicode codepoint count against
NGRAM_N rather than its byte length, matching NgramTokenizer behavior. Add a
boundary test using a multibyte two-character needle such as “éé” to verify it
falls back to a full scan instead of producing an empty exact ngram result.
---
Other comments:
In `@rust/lance-index/src/scalar/ngram.rs`:
- Line 117: Document the NGRAM_N constant with a doc comment stating that it
defines the NGram token width in characters and explaining why the width is
fixed at three (trigrams). Keep the change scoped to the constant’s
documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 9e3e682c-19e8-43d1-8888-e89da2f0ca89
📒 Files selected for processing (3)
rust/lance-index/src/scalar/expression.rsrust/lance-index/src/scalar/ngram.rsrust/lance/src/dataset/scanner.rs
| "contains" if args.len() == 2 => { | ||
| if self.index_type == "NGram" && pattern.len() < crate::scalar::ngram::NGRAM_N { | ||
| return None; | ||
| } | ||
| TextQuery::StringContains(pattern) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline rust/lance-index/src/scalar/ngram.rs --items all
rg -n -C 4 'NGRAM_N|as_bytes|chars\(|char_indices|windows\(' rust/lance-index/src/scalar/ngram.rs
rg -n -C 3 'contains\(.*é|contains\(.*NGRAM_N' rust/lance/src/dataset/scanner.rsRepository: lance-format/lance
Length of output: 8868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== expression.rs around the reviewed code =="
sed -n '890,920p' rust/lance-index/src/scalar/expression.rs | cat -n
echo
echo "== ngram.rs tokenizer-related sections =="
sed -n '90,160p' rust/lance-index/src/scalar/ngram.rs | cat -n
echo
sed -n '430,455p' rust/lance-index/src/scalar/ngram.rs | cat -n
echo
echo "== locate lance_tokenizer sources =="
fd -a 'lance_tokenizer' .
fd -a 'text_analyzer' .
fd -a 'tokenizer' rust | head -n 50
echo
echo "== search tokenizer implementation references =="
rg -n -C 3 'TextAnalyzer|tokenize_visitor|ngram' rust/lance-tokenizer rust/lance-index -g '!**/target/**'Repository: lance-format/lance
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workspace files for lance_tokenizer =="
git ls-files | rg '^rust/.*/lance_tokenizer|^rust/lance-tokenizer|^rust/.*/tokenizer'
echo
echo "== search for ngram tokenizer builder/implementation =="
rg -n -C 3 'struct .*NGram|NGramTokenizer|TextAnalyzer|tokenize_visitor|ngram' rust -g '!**/target/**' | head -n 300
echo
echo "== Cargo metadata references =="
rg -n -C 2 'lance_tokenizer' rust/Cargo.toml rust/**/Cargo.tomlRepository: lance-format/lance
Length of output: 21231
🌐 Web query:
lance_tokenizer ngram tokenizer unicode characters bytes semantics ngram_to_token TextAnalyzer
💡 Result:
In the LanceDB/Lance ecosystem, tokenization is handled by the lance-tokenizer crate [1], which provides a suite of tools for processing text into searchable tokens for Full-Text Search (FTS) indices [2][3]. Key components and concepts include: N-Gram Tokenizer: The ngram tokenizer splits text into overlapping character spans, which is effective for substring and pattern-style searching [2][3]. It supports configurable parameters such as ngram_min_length (default 3), ngram_max_length (default 3), and a prefix_only boolean flag to restrict indexing to prefix spans [4][3][5]. Unlike other tokenizers, the ngram tokenizer does not support storing token positions [5]. TextAnalyzer and Tokenizer API: The lance-tokenizer crate defines a modular architecture, including a TextAnalyzer and various Tokenizer implementations (such as NgramTokenizer, SimpleTokenizer, WhitespaceTokenizer, and RawTokenizer) [6][1]. These components allow for chaining filters like AlphaNumOnlyFilter, AsciiFoldingFilter, LowerCaser, Stemmer, and StopWordFilter to process tokens after initial segmentation [6][1]. Unicode and Semantics: Lance’s tokenizer stack is designed to handle Unicode text. While the ngram tokenizer is character-based and language-agnostic [7], other tokenizers like the ICU tokenizer provide Unicode-aware segmentation based on UAX#29 (Unicode Text Segmentation) word boundary rules [7][2]. This is particularly useful for mixed-language text or languages where whitespace is not a reliable delimiter (e.g., CJK scripts) [7][3]. Implementation: Lance recently refactored its tokenizer stack, vendoring it into the dedicated rust/lance-tokenizer crate [1]. Users generally interact with these via the base_tokenizer parameter when building scalar indices in Python, JavaScript, or Rust [2][4][8][5]. Note that base_tokenizer determines the initial splitting method, while additional processing (stemming, stop words, etc.) is handled by separate configuration parameters [4][3].
Citations:
- 1: 65ac541
- 2: https://lance.org/quickstart/full-text-search/
- 3: https://docs.lancedb.com/indexing/fts-index
- 4: https://docs.lancedb.com/search/full-text-search
- 5: https://docs.rs/lancedb/latest/lancedb/index/scalar/struct.FtsIndexBuilder.html
- 6: https://docs.rs/lance-tokenizer/latest/lance_tokenizer/all.html
- 7: feat: add UAX#29 / ICU tokenizer for multi-language FTS support #6661
- 8: https://lancedb.github.io/lancedb/js/interfaces/FtsOptions/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cargo registry candidates =="
fd -a 'tantivy' ~/.cargo/registry/src 2>/dev/null | head -n 20 || true
fd -a 'lance_tokenizer' ~/.cargo/registry/src 2>/dev/null | head -n 20 || true
echo
echo "== search for NGramTokenizer implementation in cached sources =="
rg -n -C 3 'struct NGramTokenizer|NgramTokenizer|ngram tokenizer|tokenize_visitor' ~/.cargo/registry/src -g '!**/target/**' 2>/dev/null | head -n 200 || trueRepository: lance-format/lance
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ngram search handling after tokenization =="
sed -n '438,490p' rust/lance-index/src/scalar/ngram.rs | cat -n
echo
echo "== NgramTokenizer implementation =="
sed -n '1,140p' rust/lance-tokenizer/src/ngram_tokenizer.rs | cat -n
echo
echo "== tests around contains / non-ascii / short patterns =="
rg -n -C 3 '"é|contains.*é|contains.*short|NGRAM_N|StringContains' rust/lance-index/src/scalar -g '!**/target/**'Repository: lance-format/lance
Length of output: 26030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate CodepointFrontiers =="
rg -n -C 3 'CodepointFrontiers|for_str\(' rust/lance-tokenizer rust -g '!**/target/**'
echo
echo "== inspect the definition if it exists in this repo =="
fd -a 'codepoint' rust/lance-tokenizer rust | head -n 20Repository: lance-format/lance
Length of output: 5061
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('rust/lance-index/src/scalar/ngram.rs')
lines = p.read_text().splitlines()
for start,end in [(438,490)]:
for i in range(start-1, end):
print(f"{i+1:5}: {lines[i]}")
PYRepository: lance-format/lance
Length of output: 3399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('rust/lance-index/src/scalar/ngram.rs')
lines = p.read_text().splitlines()
for start,end in [(438,470), (180,230)]:
print(f"== {start}-{end} ==")
for i in range(start-1, end):
print(f"{i+1:5}: {lines[i]}")
print()
PYRepository: lance-format/lance
Length of output: 4001
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'struct CodepointFrontiers|impl .*CodepointFrontiers|for_str\(text\)' rust/lance-tokenizer rust -g '!**/target/**'Repository: lance-format/lance
Length of output: 4071
Use codepoint count for the NGram short-query guard pattern.len() counts bytes, while NgramTokenizer splits on Unicode codepoints (CodepointFrontiers::for_str). A two-character multibyte needle like éé can slip through here and produce an exact-empty ngram result instead of falling back to a full scan. Add a non-ASCII boundary test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-index/src/scalar/expression.rs` around lines 906 - 911, Update the
NGram short-query guard in the contains expression handling to compare the
pattern’s Unicode codepoint count against NGRAM_N rather than its byte length,
matching NgramTokenizer behavior. Add a boundary test using a multibyte
two-character needle such as “éé” to verify it falls back to a full scan instead
of producing an empty exact ngram result.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Summary
An NGRAM-indexed contains(col, needle) returns zero rows when needle is shorter than the trigram width (NGRAM_N = 3), even when rows match. It should fall back to a full recheck and return the same rows as an unindexed scan.
Fixes #7841.
Root cause
For a sub-trigram needle no trigram can be derived, so NGramIndex::search returns SearchResult::at_least(RowAddrTreeMap::new()). AtLeast is documented as a lower bound (a subset of the true matches), so AtLeast(empty) means “the index knows nothing, recheck every row”. The scan path instead treated that empty lower bound as the final answer and emitted zero rows.
LIKE and regexp_match avoid this: their query parser returns None for patterns the index cannot serve, which keeps the predicate out of the index and forces a full-scan recheck. The contains path did not do this, which is why it was the only one affected.
Fix
Make the contains path in TextQueryParser return None when the index is NGram and pattern.len() < NGRAM_N, matching how LIKE/regexp_match already bypass the index. The condition is identical to the one NGramIndex::search uses (substr.len() < NGRAM_N), so the parser bypasses under exactly the case where the index would return an empty lower bound.
Changes:
Testing