Repository navigation
[fix](search) Keep numeric SEARCH leaves on the inverted index - #70488
Conversation
A SEARCH leaf on a numeric (BKD) field calls read_from_index without skip_try. When the hit count is over inverted_index_skip_threshold percent of the segment (50 by default), the iterator returns INVERTED_INDEX_BYPASS, and since SEARCH has no row path the query fails with "SEARCH cannot fall back to row execution". A dense value such as an account id that fills a segment is enough to hit it. Set skip_try for SEARCH scalar leaves so they always use the index. MATCH and plain predicates keep the bypass.
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of all three changed files at dec90f24c4e2f297947aa4557b203248745666fe: one P2 test-reliability finding, no verified production-code regression. No existing inline findings or additional user focus were supplied; live review context was refreshed.
Critical checkpoint conclusions:
- Goal:
skip_try=trueprevents the BKD hit-ratio bypass for index-only SEARCH. The flag assertion and regression cover the intended behavior, subject to the inline fixture issue. - Scope/reuse: minimal change using the existing iterator parameter; the comment explains the failure.
- Concurrency: parameter is query-local; no added threads, shared state, locks or lock ordering.
- Lifecycle/memory: both CLucene and SNII initialize in
query()independently of estimation. Bitmap shared ownership, read-only scorers and copying NULL/nested masks remain intact; no new static initialization or allocation ownership. - Configuration: no new setting; SEARCH intentionally overrides the existing threshold. The regression must pin its inherited Variant materialization budget (P2 inline).
- Compatibility: no changed symbols, storage format or wire contract; no new libc API usage. No binary ABI audit performed.
- Thrift/protobuf: unchanged; branch field-ID audit not applicable.
- Parallel paths: scalar, numeric Variant, nullable/array and nested leaves share the compiler; text SEARCH, MATCH and ordinary predicates retain their existing paths.
- Conditions/errors: no new branch; reader/query failures still propagate through Status/exception boundaries, and SEARCH still refuses row fallback.
- Test coverage: inspected high/low selectivity, plain/Variant INT, AND/OR, mixed text, standard mode, ordered LIMIT and the nested-array flag assertion. Fuzzy max-subcolumns=1 can remove the required
v.nindex, as reported inline. This suite does not claim scored TopK or every numeric type. - Expected results: 7/7/4/9/2/2 counts, descending IDs 7 and 6, and standard-mode count 4 agree with inserted rows. No expected-output files changed. Assertion/table-name/final-DROP conventions differ from current repository guidance; no additional functional defect was established for those patterns.
- Observability: existing reader/SEARCH timings, hit-row profiling and error diagnostics remain sufficient for this flag change.
- Persistence/failover: production code is read-only; no metadata or EditLog changes.
- Data writes/atomicity: segment visibility, transaction selection and MoW delete-bitmap filtering are unchanged.
- FE/BE propagation: no transmitted variable added or changed.
- Performance: removes an estimation pass for SEARCH; high-hit index materialization is required because row execution is unavailable. No measurements performed.
- Final sweep: all changed paths, candidate dispositions and initial risk items reviewed; reader initialization, cached-bitmap mutation and DSL-cache masking concerns dismissed with source evidence. No unresolved code-level coverage gap.
Validation is static inspection only: no builds, tests, benchmarks or live query reproduction were run.
Review converged after 2 round(s).
| CREATE TABLE ${tableName} ( | ||
| `id` INT NOT NULL, | ||
| `k` INT NULL, | ||
| `v` variant NULL, |
There was a problem hiding this comment.
[P2] Pin the Variant subcolumn budget for this fixture
The p0 pipeline enables fuzzy session variables, and SessionVariable.initFuzzyModeVariables() can leave default_variant_max_subcolumns_count=1 with nested groups disabled. This untyped VARIANT inherits that budget even after the suite disables doc mode. Both n and p occur in all ten rows, so select_variant_paths() materializes only p and sends n to sparse storage. The first search('v.n:5') then fails with SEARCH index unavailable for Variant field 'v.n', independently of the threshold fix, making this new p0 test flaky. Pin the budget to at least 2 (or 0 for unlimited) before CREATE, or declare both paths explicitly.
There was a problem hiding this comment.
Pinned default_variant_max_subcolumns_count = 2048 before the CREATE in afc9493. Checked locally with the global set to 1: the old version fails with “SEARCH index unavailable for Variant field ‘v.n’”, the new one passes.
…hreshold The p0 pipeline can fuzz default_variant_max_subcolumns_count down to 1, which would push v.n to sparse storage and fail the suite for an unrelated reason. Pin it to 2048, and follow the regression test conventions: hardcoded table name, qt_ checks with an .out file, and no DROP at the end.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review completed against the exact PR head afc9493 (base 9e598d3) after two bounded rounds. The BE change is correctly scoped: ScalarLeafCompiler supplies skip_try only for SEARCH scalar leaves, and InvertedIndexIterator uses it only to suppress the BKD threshold preflight that would otherwise become SEARCH cannot fall back to row execution. Exact BKD query execution, bitmap/query caches, null masking, boolean composition, and scoring remain on their existing paths. The unit test verifies flag propagation, and no include or unity issue was found. One P2 test-quality finding is included inline: the new regression enables FunctionSearch's whole-SEARCH DSL cache, so repeated mixed Variant queries can return before leaf compilation; with only 10 rows in one 128-point leaf, the first estimate can equal the strict 50% threshold. The Variant/mixed path can therefore pass on the pre-fix code even though the plain INT case may still catch it. The existing P2 thread 4545779272 is resolved by the current 2048 subcolumn budget and is not duplicated; no existing P0/P1 blocker remains. The user focus file contains no additional focus. Review evidence is static only; no builds or tests were run. All assigned reviewers returned NO_NEW_VALUABLE_FINDINGS in round 2, so this review converged.
Review converged after 2 round(s).
| // the index. | ||
| suite("test_search_bkd_skip_threshold", "p0") { | ||
| sql """ set enable_common_expr_pushdown = true """ | ||
| sql """ set enable_inverted_index_query_cache = true """ |
There was a problem hiding this comment.
[P2] Make this regression bypass the whole-SEARCH cache while testing the leaf threshold. With enable_inverted_index_query_cache=true, mixed queries such as v.n:5 AND v.p:software are served from FunctionSearch's DSL bitmap cache on the second warmup, before ScalarLeafCompiler::read_from_index runs. The fixture has only 10 rows, so the first estimate may be half of one 128-point BKD leaf (5), which is exactly the 50% boundary and does not trigger the old strict > check. The plain k:5 case can still catch the bug, but the Variant/mixed path this test claims to cover may pass on the pre-fix code. Disable the DSL cache for the warmup or use enough rows / a lower threshold to force a pre-fix failure for each path.
### Background `search()` only runs through the inverted index; there is no row-by-row path, and `vsearch.cpp` turns any index bypass into "SEARCH cannot fall back to row execution" (added in #66052). For numeric fields (BKD readers), `InvertedIndexIterator::read_from_index` first estimates the hit count and returns `INVERTED_INDEX_BYPASS` when it is over `inverted_index_skip_threshold` percent of the segment (50 by default), so the predicate can be evaluated on rows instead. That bypass is useful for normal predicates and MATCH, not for SEARCH. ### What problem does this PR solve? Issue Number: None Related PR: #64782 (same fix for the pre-refactor code, closed without merge), #68758 (moved the scalar leaf into `ScalarLeafCompiler`, bug kept) PR Type: Bugfix Problem Summary: A SEARCH term on a numeric field fails when the value matches more than half of a segment, for example an account id on a table loaded per account: ``` SELECT count(*) FROM t WHERE search('v.account_id:8069') ERROR: [E-6001]SEARCH cannot fall back to row execution: [E-6004]hit count: 350255, bkd inverted reached limit 50% , segment num rows:500000 ``` `ScalarLeafCompiler::compile` now sets `skip_try` so SEARCH leaves always read the index. MATCH and plain predicates keep the bypass. ### Details - **Frequency:** Any SEARCH term on a numeric column or numeric variant subcolumn whose value covers more than `inverted_index_skip_threshold` percent of a segment. Common for tenant or account ids. On 4.1 it often shows up only on the second run, once the exact hit count comes from the query cache. - **Root cause:** `ScalarLeafCompiler::compile` calls `read_from_index` with `skip_try = false`, so the BKD hit-ratio check applies to SEARCH even though SEARCH cannot evaluate rows. - **Why existing tests missed it:** No SEARCH test runs a numeric term that covers more than half a segment with the default threshold; `test_search_inverted_is_null_pushdown` sets `inverted_index_skip_threshold=0`, which turns the bypass off. - **Fix:** Set `param.skip_try = true` before `read_from_index` in `ScalarLeafCompiler::compile`. This is the only `read_from_index` call on the SEARCH path. - **Other paths and branches:** branch-4.1 has the same bug in `FunctionSearch::build_leaf_query`; a separate PR picks this there. Numeric range clauses (`field:[a TO b]`) are still unsupported in SEARCH; that is not changed here. ### Testing - New regression test `search/test_search_bkd_skip_threshold`: a MOW table where one value covers 70% of the segment, on a variant subcolumn and on a plain INT column, alone, combined with a text term, in standard mode and with ORDER BY ... LIMIT, each run 3 times with the query cache on. Fails before the fix with "SEARCH cannot fall back to row execution", passes after. Ran locally against a cluster built from this branch, and against the same commit without the fix. - `FunctionSearchTest.TestBuildLeafQueryVariantNestedIntUsesDirectIndexReader` now checks that the scalar leaf passes `skip_try = true` to the iterator. Compiled locally with the BE unit-test flags; not run locally (no BE UT build), left to CI. - Manual timing on 2M rows (index query cache and SQL cache off, median of 11): a 20% numeric match 10 ms before vs 12 ms after (noise), the 70% match fails before and takes 16 ms after; text SEARCH and plain filters unchanged. ### Release note Fix `search()` failing with "SEARCH cannot fall back to row execution" when a term on a numeric field matches more than `inverted_index_skip_threshold` percent of a segment. ### Check List (For Author) - Behavior changed: Yes (see Details). SEARCH no longer follows `inverted_index_skip_threshold` for numeric fields; queries that failed now return results. - Does this need documentation: No - Pre-submission status: Partially verified: the BE unit test was compiled but not run locally. --------- Co-authored-by: Shivam Gupta <shivam.gupta@recruitcrm.io>
Background
search()only runs through the inverted index; there is no row-by-row path, andvsearch.cppturns any index bypass into "SEARCH cannot fall back to row execution" (added in #66052). For numeric fields (BKD readers),InvertedIndexIterator::read_from_indexfirst estimates the hit count and returnsINVERTED_INDEX_BYPASSwhen it is overinverted_index_skip_thresholdpercent of the segment (50 by default), so the predicate can be evaluated on rows instead. That bypass is useful for normal predicates and MATCH, not for SEARCH.What problem does this PR solve?
Issue Number: None
Related PR: #64782 (same fix for the pre-refactor code, closed without merge), #68758 (moved the scalar leaf into
ScalarLeafCompiler, bug kept)PR Type: Bugfix
Problem Summary:
A SEARCH term on a numeric field fails when the value matches more than half of a segment, for example an account id on a table loaded per account:
ScalarLeafCompiler::compilenow setsskip_tryso SEARCH leaves always read the index. MATCH and plain predicates keep the bypass.Details
inverted_index_skip_thresholdpercent of a segment. Common for tenant or account ids. On 4.1 it often shows up only on the second run, once the exact hit count comes from the query cache.ScalarLeafCompiler::compilecallsread_from_indexwithskip_try = false, so the BKD hit-ratio check applies to SEARCH even though SEARCH cannot evaluate rows.test_search_inverted_is_null_pushdownsetsinverted_index_skip_threshold=0, which turns the bypass off.param.skip_try = truebeforeread_from_indexinScalarLeafCompiler::compile. This is the onlyread_from_indexcall on the SEARCH path.FunctionSearch::build_leaf_query; a separate PR picks this there. Numeric range clauses (field:[a TO b]) are still unsupported in SEARCH; that is not changed here.Testing
search/test_search_bkd_skip_threshold: a MOW table where one value covers 70% of the segment, on a variant subcolumn and on a plain INT column, alone, combined with a text term, in standard mode and with ORDER BY ... LIMIT, each run 3 times with the query cache on. Fails before the fix with "SEARCH cannot fall back to row execution", passes after. Ran locally against a cluster built from this branch, and against the same commit without the fix.FunctionSearchTest.TestBuildLeafQueryVariantNestedIntUsesDirectIndexReadernow checks that the scalar leaf passesskip_try = trueto the iterator. Compiled locally with the BE unit-test flags; not run locally (no BE UT build), left to CI.Release note
Fix
search()failing with "SEARCH cannot fall back to row execution" when a term on a numeric field matches more thaninverted_index_skip_thresholdpercent of a segment.Check List (For Author)
inverted_index_skip_thresholdfor numeric fields; queries that failed now return results.