Repository navigation
feat: add TEXT NGRAM indexes, Grep index routing, and max-count - #55136
junjiejiangjjj wants to merge 1 commit into
Conversation
sre-ci-robot
added
area/compilation
area/test
sig/testing
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
mergify
Bot
added
dco-passed
|
[ci-v2-notice] To rerun ci-v2 checks, comment with:
If you have any questions or requests, please contact @zhikunyao. |
liliu-z
left a comment
liliu-z
left a comment
There was a problem hiding this comment.
The TEXT admission part looks fine to me. My main concern is scope. The description says Grep index routing is not implemented here, but the diff already implements it: Rust literal-condition extraction, the cgo proto, INgramReader::ConditionTreeCandidates, and Index/Probe routing in SegmentGrep.cpp. It also adds a new user-facing -m/--max-count option and a new function.grep.probeMatchRatio config.
Routing is on by default, with no switch, for every grep on a sealed segment whose field has a whole-field NGRAM index. That includes existing VARCHAR NGRAM fields, not just TEXT. A gap in the necessary-condition extraction would silently drop matching rows, so reviewers need to know this is in the PR. Could we split it into (1) TEXT NGRAM admission, (2) grep NGRAM routing, and (3) -m? At minimum, please update the title and description.
Also, ruff format --check fails on tests/python_client/testcases/indexes/test_ngram.py.
| if strings.HasPrefix(value, "--") { | ||
| // Value-taking flags consume the next token even if it resembles an option. | ||
| name, attached, hasValue := strings.Cut(value, "=") | ||
| if name == "--max-count" { |
There was a problem hiding this comment.
-m/--max-count is new user-facing grep syntax and has nothing to do with NGRAM. The SDK passes the args string through as-is, so this becomes a public contract once it ships. Could it go in its own PR with its own description?
| } | ||
| p.GrepBatchSize.Init(base.mgr) | ||
|
|
||
| p.GrepProbeMatchRatio = ParamItem{ |
There was a problem hiding this comment.
Do we have numbers showing Probe helps on the target workload? The design doc says the 5% default isn't validated, and the perf content was removed. If I read condition.rs right, -i -e error already expands to an OR of 32 five-byte literals (MAX_SEQUENCES). So most case-insensitive queries with limit <= 128 would take this path, not just rare wide-OR patterns. Without data, I'd ship Index-only routing first and add Probe and this config later. Once a config is exported, it's hard to remove.
| persisted index format, or error-code projection was changed. No performance | ||
| speedup is claimed by this correctness run. | ||
|
|
||
| Evidence: `/tmp/grep-max-count-e2e-q5nn27kt/verification.json`, `e2e-report.json`, |
There was a problem hiding this comment.
nit: these /tmp/... paths only exist on your machine and won't mean anything to readers. The same goes for §8 "Documentation Corrections" in the new doc. I'd drop both and keep the docs about the design itself.
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 10.9min | - |
| ❌ Code-Check | FAILURE | 2.3min | - |
| ❌ UT-Integration | FAILURE | 92.9min | - |
| ✅ UT-GO | SUCCESS | 36.8min | - |
| ✅ UT-CPP-Cov | SUCCESS | 42.1min | 22969 total, 22969 passed, 0 failed |
Total: 113min | Pipeline | Artifacts
Overall Coverage: 78.7%
Diff Coverage: CPP 96.4% (1294 hit, 48 miss, 1342 measurable lines, 768 unmeasured) | Go 96.5% (55 hit, 2 miss, 57 measurable lines, 390 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage is partial; 390 changed lines were unmeasured.
Total Patch Coverage: 96.4% (1349/1399 measurable lines, 1158 unmeasured)
Failed Test Logs:
- Code-Check: download log archive (
code-check.loginside; index) - UT-Integration: download log archive (
ut-integration-test.loginside; index)
287f3aa to
7e4118f
Compare
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 8.4min | - |
| ❌ Code-Check | FAILURE | 3.7min | - |
| ✅ UT-Integration | SUCCESS | 42.5min | - |
| ✅ UT-GO | SUCCESS | 24.3min | - |
| ✅ UT-CPP-Cov | SUCCESS | 40.9min | 22969 total, 22969 passed, 0 failed |
Total: 58min | Pipeline | Artifacts
Overall Coverage: 78.7%
Diff Coverage: CPP 96.4% (1294 hit, 48 miss, 1342 measurable lines, 768 unmeasured) | Go 96.5% (55 hit, 2 miss, 57 measurable lines, 390 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage is partial; 390 changed lines were unmeasured.
Total Patch Coverage: 96.4% (1349/1399 measurable lines, 1158 unmeasured)
Failed Test Logs:
- Code-Check: download log archive (
code-check.loginside; index)
7e4118f to
4cb8d15
Compare
✅ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 16.7min | - |
| ✅ Code-Check | SUCCESS | 7.8min | - |
| ✅ UT-Integration | SUCCESS | 58.5min | - |
| ✅ UT-GO | SUCCESS | 41.2min | - |
| ✅ UT-CPP-Cov | SUCCESS | 81.5min | 22969 total, 22969 passed, 0 failed |
Total: 132min | Pipeline | Artifacts
Overall Coverage: 78.7%
Diff Coverage: CPP 96.4% (1291 hit, 48 miss, 1339 measurable lines, 769 unmeasured) | Go 96.0% (48 hit, 2 miss, 50 measurable lines, 369 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage is partial; 369 changed lines were unmeasured.
Total Patch Coverage: 96.4% (1339/1389 measurable lines, 1138 unmeasured)
4cb8d15 to
02c0070
Compare
junjiejiangjjj
changed the title
|
@liliu-z Updated the title and description to cover TEXT NGRAM support, Grep index routing, and |
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ❌ Build | FAILURE | 5.2min | - |
| ❌ Code-Check | SKIPPED | - | - |
| ❌ UT-Integration | SKIPPED | - | - |
| ❌ UT-GO | SKIPPED | - | - |
| ❌ UT-CPP-Cov | SKIPPED | - | - |
Total: 10min | Pipeline | Artifacts
Failed Test Logs:
- Build: download log archive (
build.loginside; index)
02c0070 to
f5b9668
Compare
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 16.9min | - |
| ✅ Code-Check | SUCCESS | 12.7min | - |
| ❌ UT-Integration | UNKNOWN | - | - |
| ✅ UT-GO | SUCCESS | 40.6min | - |
| ❌ UT-CPP-Cov | FAILURE | 48.9min | - |
Total: 77min | Pipeline | Artifacts
Diff Coverage: Go 96.0% (48 hit, 2 miss, 50 measurable lines, 369 unmeasured)
Go Patch Warning: WARNING: Go patch coverage is partial; 369 changed lines were unmeasured.
Total Patch Coverage: 96.0% (48/50 measurable lines, 369 unmeasured)
Failed Test Logs:
- UT-CPP-Cov: download log archive (
cpp-ut-cov.loginside; index)
Allow explicit NGRAM indexes on TEXT fields and preserve the index type when scalar autoindex is enabled. Keep TEXT AUTOINDEX and other user-created scalar index types unsupported. Add admission, parameter validation, native TEXT/LOB build-load tests, and a standalone verification script. Verify metadata persistence, DataNode builds, and raw TEXT retrieval across reload and restart. Shutdown issues remain unresolved. This establishes the index prerequisite for Grep acceleration. Follow-up work will extract necessary literal conditions from Rust regexes, use NGRAM to filter candidate offsets before reading TEXT/LOB values, and retain the Grep matcher for exact refinement. Grep index routing is not implemented in this change. Signed-off-by: junjie.jiang <junjie.jiang@zilliz.com>
f5b9668 to
95bbfd3
Compare
liliu-z
left a comment
liliu-z
left a comment
There was a problem hiding this comment.
Thanks for updating the title/description and making the ratio an internal constant. I re-checked the extraction (condition.rs) and the C++ planning (PlanNgramRegexCondition) for the direction of every fallback: AND only drops children, OR turns into True as soon as any branch does, budget overflows weaken to True, and the HIR is built with the same case/Unicode settings as the matcher. I couldn't find a way for a real match to fall outside the candidate set, so no correctness blocker from me. What's left is mostly Probe and the default-on performance, see inline.
| } | ||
|
|
||
| bool | ||
| ShouldProbe(const GrepIndex& index, int64_t limit) { |
There was a problem hiding this comment.
Thanks for adding the numbers. To me they argue for leaving Probe out for now. In §7.3 Probe is slower than No-Probe on all three TEXT corpora (Go 10.09 vs 9.19 ms, Python 11.17 vs 9.99, Linux 45.55 vs 34.26) and roughly even on VARCHAR, and the c=16 runs in §7.4 show no throughput gain. The clear win only shows up on the 60%-hit severity fixture.
The trigger is also somewhat accidental. 32 leaves is the same number as MAX_SEQUENCES, so for -i it comes down to word length: -i error, -i panic and -i failed take Probe, while -i warn, -i timeout and -i exception go straight to Index (I checked with condition.rs against regex-syntax 0.8.11). I'd ship direct Index routing first and bring Probe back once there are route counters, as §7.1 already suggests. §6.2 and §7.1/7.3/7.4 could go with it.
| segment->HasFieldData(field) && | ||
| (!snapshot || view_column != nullptr); | ||
|
|
||
| auto index = PrepareGrepIndex(segment, &op_ctx, meta, plan, size, rows); |
There was a problem hiding this comment.
Routing is on for every eligible sealed segment, including existing VARCHAR NGRAM fields, and §7.2 shows the 10K short-text set at c=32 dropping from 1391 to 709 QPS (p50 20.9 → 37.8 ms). The cause hasn't been profiled yet. Could we find out where that time goes before this becomes the default? Until then it's hard to say whether short-text collections should take this path at all.
| // queryScanNativeSegment borrows the exact native handle retained by retrieval. | ||
| // The caller owns the legacy segment pin or QueryView lease through scanning, | ||
| // identity retrieval and final materialization; this helper must not release it. | ||
| func queryScanNativeSegment(segment Segment) (segcore.CSegment, SegmentType, error) { |
There was a problem hiding this comment.
If I read it right, this also makes grep work on QueryView-served segments (sealedReadState and SN growing views). Before, they failed with QueryScan requires a local segcore segment. That's a separate fix and it isn't in the description. Could you mention it there and add a small test? The s.native vs parent.csegment choice is easy to break later.
|
|
||
| The four-corpus threshold aggregate favors 0.25 over 0.05 by about 3.35% at p50 and 3.46% at p95, but individual workloads regress, including HDFS `termi`, limit 32, with about 23.6% higher cross-round geometric-mean p95. This does not establish a better general default. No-Probe improves the source-code aggregates while losing on HDFS and on several high-hit early-return subsets. The evidence supports workload-dependent routing, not a universal Probe speedup or a TEXT-specific limit cutoff. | ||
|
|
||
| Entry points: [corpus benchmark](../../../tests/scripts/grep_probe_corpus.py), [threshold report](../../../tests/scripts/grep_probe_corpus_report.py), and [No-Probe comparison](../../../tests/scripts/grep_probe_compare_no_probe.py). Local raw evidence is retained under `/tmp/grep-probe-multi-corpus-20261105` and `/tmp/grep-no-probe-20261105`; these are local artifacts, not repository-published datasets. |
There was a problem hiding this comment.
nit: the /tmp/... paths are still here and in §7.4. The linked tests/scripts/grep_probe_*.py and grep_severity_probe_ab.py aren't in this PR either, so those links are broken. I'd either add the scripts or drop the references.
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 21.3min | - |
| ✅ Code-Check | SUCCESS | 12.9min | - |
| ❌ UT-Integration | UNKNOWN | - | - |
| ✅ UT-GO | SUCCESS | 39.9min | - |
| ❌ UT-CPP-Cov | FAILURE | 48.0min | - |
Total: 81min | Pipeline | Artifacts
Diff Coverage: Go 79.7% (63 hit, 16 miss, 79 measurable lines, 375 unmeasured)
Go Patch Warning: WARNING: Go patch coverage is partial; 375 changed lines were unmeasured.
Total Patch Coverage: 79.7% (63/79 measurable lines, 375 unmeasured)
Failed Test Logs:
- UT-CPP-Cov: download log archive (
cpp-ut-cov.loginside; index)
This PR adds TEXT NGRAM index support, NGRAM-backed Grep routing for sealed VARCHAR and TEXT fields, and the user-facing
-m/--max-countGrep option.Explicit NGRAM indexes are accepted on TEXT fields, including when scalar autoindex is enabled, while preserving the explicit NGRAM index type. The change includes parameter validation, the existing index persistence/build/load path, native TEXT/LOB tests, and reload/restart verification. TEXT AUTOINDEX and other user-created scalar index types remain unsupported.
Grep routing is implemented in this PR and is enabled automatically for eligible sealed VARCHAR and TEXT fields with a whole-field NGRAM index, including existing VARCHAR NGRAM fields. Rust extracts necessary literal conditions from regexes, transfers the condition tree through Protobuf and the C API, and C++ uses
INgramReader::ConditionTreeCandidatesto obtain candidate offsets. The exact Rust matcher verifies original field values and produces match details. The required invariant is that every actual match remains an index candidate.Routing works as follows:
The included performance results compare Probe with direct Index routing and document workload-dependent benefits and regressions. They do not establish a universal Probe speedup or an optimal general-purpose threshold.
The new
-m/--max-countoption caps matching lines independently for each queried field value. It is distinct from the Query record limit and is carried through the Go parser, native boundary, and Rust matcher. The last occurrence wins; zero returns no matches after pattern validation and compilation.Design documents:
issue: #53020