Repository navigation
[fix](ngram-bf) Give up the NGRAM index when a LIKE pattern can carry an escape - #68021
Merged
csun5285 merged 1 commit intoSep 16, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review outcome
REQUEST_CHANGES — complete and converged after three review rounds. The escape gate is conservative for the Release path that reaches it, but the PR is not ready because the custom-ESCAPE integration aborts in checked builds, the regression does not prove the intended NGRAM eligibility behavior, and mixed same-column LIKE predicates lose otherwise valid NGRAM pruning. No additional user-provided focus was supplied; the whole PR was reviewed.
Findings
- [P1] Checked-build integration: a three-argument LIKE reaches an upstream arity-two
DCHECKbeforehas_custom_escapecan be consulted. - [P2] Test oracle: result equality passes even if NGRAM is disabled for every LIKE, so neither preserved escape-free pruning nor escaped fallback is demonstrated.
- [P2] Performance: an ineligible escaped/custom LIKE makes the aggregate same-column predicate ineligible, suppressing an eligible sibling's query bloom filter.
Critical checkpoint conclusions
- Goal and correctness: Rejecting raw-backslash/custom-escape patterns is a sound conservative direction for reachable positive LIKE predicates, but the checked-build arity failure means the custom-escape path is not integrated end to end; the goal is therefore only partially accomplished.
- Scope and clarity: The diff is small and locally clear, but the necessary upstream arity handling and downstream aggregate-eligibility interaction are outside the current patch.
- Concurrency and lifecycle: No new thread or lock interaction is introduced. Scanner clones own independent query bloom filters, while the retained expression context keeps the pattern and LIKE state alive; no race, stale reuse, or ownership defect was found.
- Configuration: No configuration item is added. The existing query-LIKE bloom-filter switch is read during reader initialization; no new dynamic-update issue was found.
- Compatibility and parallel paths: No symbol, Thrift, storage-format, persistence, or rolling-upgrade change is involved. The sole production query-BF creator/setter and the segment evaluation gate were traced; no bypass path was found.
- Conditional logic: The backslash/custom-escape rejection is conservative, including UTF-8 and short-pattern cases. Its same-column AND interaction causes the reported pruning regression. A proposed NOT-LIKE concern was dismissed because the current normalization shape rejects it before
LikeColumnPredicatecreation. - Tests and results: The new SQL cases have intended runtime escape bytes, matching fixtures, and ordered comparisons, but they lack a deterministic path-use assertion and hit the checked-build arity assertion for custom
ESCAPE. This was a static-only review: the review contract prohibited builds/tests, so no independent execution result is claimed. - Observability: Existing
RowsBloomFilterFilteredprofile data is sufficient to build a deterministic regression oracle; no production logging/metric addition is otherwise required. - Transactions, persistence, and writes: Not applicable; the change affects read-time predicate/index pruning only.
- FE/BE propagation: No new transmitted variable is added. Existing FE preservation of the third LIKE argument is exactly what exposes the BE arity mismatch.
- Performance: The
string_viewcheck avoids a copy, but the mixed-conjunction aggregate gate can turn a selective page-pruned scan into an unpruned scan, as reported inline. No other CPU, memory-accounting, or allocation concern was found. - Other review areas: Error/status handling, memory safety, nullability, data visibility/versioning, delete bitmap behavior, Cloud/local behavior, and static initialization are unchanged or not applicable.
All accepted candidates are included inline; no unresolved suspicious point remains.
| return false; | ||
| } | ||
| // A pattern that can carry an escape is not supported by the ngram index. | ||
| if (_state->has_custom_escape) { |
Contributor
There was a problem hiding this comment.
[P1] Make three-argument LIKE reach this gate in checked builds
This new branch relies on a custom-ESCAPE LIKE reaching LikeColumnPredicate, but the upstream OlapScanLocalState::_should_push_down_function_filter still executes DCHECK(children.size() == 2) (olap_scan_operator.cpp:544). Nereids preserves the third escape argument, and the new cases at lines 55-57 enable function pushdown, so ASAN/Debug builds (which do not define NDEBUG) abort before has_custom_escape can be consulted. Please make the normalizer explicitly accept/validate both two- and three-argument LIKE (or reject three-argument pushdown cleanly) so these cases actually exercise the fix in checked builds.
| '中文测试行', | ||
| ] | ||
|
|
||
| // [pattern, escape clause]. The escape-free entries must keep using the index; the rest are |
Contributor
There was a problem hiding this comment.
[P2] Assert the NGRAM path, not only result equality
The stated invariant here is two-sided: escaped/custom patterns must bypass NGRAM, while escape-free patterns must keep using it. The assertions below only compare result rows, so an implementation that returns false from can_do_bloom_filter(true) for every LIKE passes the entire suite, including %plain% and %中文%. Please add a deterministic path-use oracle—for example the existing RowsBloomFilterFiltered profile technique from test_ngram_bloomfilter_index_change.groovy (with controlled page/compaction layout)—that proves positive pruning for an escape-free control and no BF pruning for the escaped/custom cases.
| const auto* tablet_index = _tablet_schema->get_ngram_bf_index(col.unique_id()); | ||
| if (tablet_index && config::enable_query_like_bloom_filter) { | ||
| if (tablet_index && config::enable_query_like_bloom_filter && | ||
| predicate->can_do_bloom_filter(true)) { |
Contributor
There was a problem hiding this comment.
[P2] Preserve safe LIKE pruning inside mixed conjunctions
This per-predicate false is later aggregated by AndBlockColumnPredicate::can_do_bloom_filter(true), which requires every same-column predicate to be eligible. Thus a safe v LIKE '%plain%' combined with a backslash-bearing LIKE causes the latter to disable the NGRAM index for the safe predicate too, even though this method already built its query bloom filter. On large scans that turns a selective page-pruned query into an unpruned scan. Please keep ineligible LIKEs out of the NGRAM predicate group, or otherwise let eligible children drive BF evaluation while ineligible children accept, so only the unsafe predicate loses the index.
yiguolei
previously approved these changes
Sep 16, 2026
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16893 ms |
Contributor
TPC-DS: Total hot run time: 82844 ms |
Contributor
ClickBench: Total hot run time: 14.88 s |
… an escape
The NGRAM_BF index prunes a page whose bloom filter misses a token of the LIKE
pattern, so every token derived from the pattern has to occur in all strings
that pattern matches. Deriving them means reading the pattern's escapes exactly
the way LIKE reads them, and the storage layer did not.
Two ways this dropped rows, both silently:
- `LIKE 'C:\\U%'` (a backslash in front of an ordinary character). The token
extractor drops that backslash, asking for `:U`, while LIKE keeps it as a
literal and every matching string holds `:\U`.
- `LIKE '%a!%b%' ESCAPE '!'`. The pushdown path never captures the escape
character, so the extractor reads `!` as an ordinary character and `%` as a
wildcard, asking for `a!`, which no matching string has to contain.
Rather than teach the extractor another reading of LIKE's escapes, report the
predicate as unable to use an ngram bloom filter whenever an escape can be in
play. LIKE already reads escapes in three places that do not agree with each
other -- convert_like_pattern, the equals/prefix fast-path regexes, and the
custom ESCAPE clause -- so a fourth reader here is a standing source of silently
wrong results, and the pruning it buys covers only patterns that spell out a
backslash. The test is deliberately free of escape semantics: a backslash
anywhere in the pattern, or an ESCAPE clause at all. An escape-free pattern
cannot reach the extractor's escape branches at all, so it keeps the index and
the pruning it had, which is the common case even over data full of backslashes.
`LIKE ... ESCAPE 'c'` reaches the scan as a three-argument call, and the arity
assertion on the pushdown path aborted a checked build before the predicate was
ever built. Drop it: the parser already requires the third argument to be a
string literal, so it is never the slot the loop below looks for.
The regression suite compares indexed and unindexed results for backslash,
custom-ESCAPE and escape-free patterns, and reads RowsBloomFilterFiltered from
the profile so that equal result sets cannot pass by having switched the index
off everywhere: an escape-free pattern must still prune, an escaped one must
not. Every row is inserted by its own statement, because a page that also holds
an unrelated row can carry the very token a mis-read pattern asks for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EDn3iyWLz47DBPQBb9NhUq
csun5285
force-pushed
the
fix-DORIS-28978-ngram-like-escape
branch
from
September 16, 2026 03:40
a4c0d1f to
b4727cd
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Sep 19th, 2026 8:24 AM. The selected account is excluded until 2026-09-19T08:24:00Z. Please trigger /review again; another configured account may be available. |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review outcome
COMMENT — complete and converged after one review round. I found no new blocking or correctness issue at head b4727cda980b63eabcb7735136d05e496bb4beaa. The checked-build three-argument integration and the positive/negative NGRAM path oracle raised on the prior head are addressed. The already-threaded mixed same-column conjunction behavior remains a conservative, author-accepted performance tradeoff, so it is not duplicated here.
No additional user-provided focus was supplied; the entire four-file PR was reviewed.
Critical checkpoint conclusions
- Goal, correctness, and proof: The change meets its goal by bypassing NGRAM pruning for every custom-
ESCAPELIKE and every default-escape pattern containing a raw backslash, while preserving pruning for escape-free patterns. For the accepted set, the tokenizer emits only ngrams from mandatory literal runs, so matching rows cannot be falsely pruned. The regression compares indexed results with a no-index control and separately proves both positive escape-free pruning and zero pruning for unsafe patterns. - Scope and clarity: The production change is small and focused: one legal-arity integration adjustment, one predicate-local eligibility rule, and one reader-side gate. The direct
<string_view>include supports the allocation-free byte check. - Concurrency and lifecycle: No new thread, lock, or atomic interaction is introduced. Retained expression contexts own the constant pattern and immutable LIKE state; each scanner clone owns separate Hyperscan scratch and builds/attaches its own query Bloom filter before segment evaluation.
- Configuration: No configuration item is added or changes semantics. The existing mutable query-LIKE Bloom-filter switch is consulted at reader initialization as before.
- Compatibility and parallel paths: There is no wire, symbol, storage-format, persistence, or rolling-upgrade change. Nereids' legal two-/three-argument shapes and the legacy two-argument shape were traced; Cloud and shared-nothing scans reuse the same tablet-reader path. Ordinary and NGRAM Bloom-filter selection remain disjoint.
- Conditional logic: Nereids requires a third escape argument to be a one-byte string literal, so legal child 2 cannot trigger the
children[1 - i]access.NOT LIKEstays residual at the outer compound-predicate slot gate and therefore cannot construct an opposite NGRAM storage predicate. Empty, wildcard-only, and too-short literal runs safely attach no query token. - Tests and results: Groovy/SQL escaping, all matching fixtures, ordered comparisons, UUID profile lookup, counter attribution, compaction/page coalescing, and always-enable/always-disable failure modes were checked. This review was static-only because the review contract prohibited local builds/tests. Current PR checks report compile, BE unit, and P0 regression success; I did not independently execute them.
- Observability: Existing
RowsBloomFilterFilteredprofile data is sufficient for the regression oracle; no production metric or logging addition is needed. - Transactions, persistence, and writes: Not applicable. The change affects read-time predicate/index pruning only and does not alter visibility, delete bitmaps, or write atomicity.
- FE/BE propagation: No new transmitted variable is introduced. Existing FE preservation of the third LIKE argument is handled by the adjusted BE normalizer and existing
FunctionLikestate. - Performance: Escape-free LIKE retains its allocation-free NGRAM path. Unsafe patterns conservatively lose pruning. The known case where one unsafe predicate suppresses an eligible same-column sibling is already covered by
discussion_r4015549765and explicitly accepted by the author; no distinct performance defect was found. - Other areas: Error/status propagation, memory accounting, nullable handling, static initialization, security boundaries, and test-output compatibility are unchanged or not applicable.
yiguolei
approved these changes
Sep 16, 2026
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 17110 ms |
Contributor
TPC-DS: Total hot run time: 81662 ms |
Contributor
ClickBench: Total hot run time: 14.67 s |
3 of 9 tasks
yiguolei
pushed a commit
that referenced
this pull request
Sep 22, 2026
…rn can carry an escape (#68021) (#68319) ### What problem does this PR solve? Related PR: #68021 Backport #68021 to `branch-4.1` to prevent NGRAM Bloom Filter pruning from dropping matching rows when a LIKE pattern contains a backslash or uses a custom ESCAPE. Escape-free patterns remain eligible for the index. Cherry-picked from `2fe54ad3516e0638444f91f4581d1127bb48a411`. One conflict was resolved in `TabletReader::_init_conditions_param`: retain branch-4.1's `function_filters` loop and `is_like_predicate(pred)` check, and add `pred->can_do_bloom_filter(true)` to the existing index initialization guard. The other BE changes and the regression suite are unchanged from the original patch. ### Validation - `git diff --check` passed; no unresolved conflicts. - Verified the patches for `olap_scan_operator.cpp` and `like_column_predicate.h` match the original commit. - Verified `test_ngram_bloomfilter_index_like_escape.groovy` is identical to the original commit. - Confirmed branch-4.1 already has `LikeState::has_custom_escape`. - Compilation and runtime regression tests were not run in this session; CI validation remains pending. ### Release note Fix incorrect LIKE query results with NGRAM Bloom Filter indexes when the pattern contains an escape. ### Check List (For Author) - Test - [x] Regression test included from #68021 (not executed in this session) - [ ] Unit Test - [ ] Manual test - Behavior changed: - [x] Yes. Skip NGRAM Bloom Filter pruning for LIKE patterns that can carry an escape. - Does this need documentation? - [x] No. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label Co-authored-by: Chenyang Sun <sunchenyang@selectdb.com>
yiguolei
added
dev/4.1.5-merged
dev/4.2.x
and removed
dev/4.1.x-conflict
dev/4.2.x
labels
yiguolei
pushed a commit
that referenced
this pull request
Sep 22, 2026
…rn can carry an escape (#68021) (#68319) ### What problem does this PR solve? Related PR: #68021 Backport #68021 to `branch-4.1` to prevent NGRAM Bloom Filter pruning from dropping matching rows when a LIKE pattern contains a backslash or uses a custom ESCAPE. Escape-free patterns remain eligible for the index. Cherry-picked from `2fe54ad3516e0638444f91f4581d1127bb48a411`. One conflict was resolved in `TabletReader::_init_conditions_param`: retain branch-4.1's `function_filters` loop and `is_like_predicate(pred)` check, and add `pred->can_do_bloom_filter(true)` to the existing index initialization guard. The other BE changes and the regression suite are unchanged from the original patch. ### Validation - `git diff --check` passed; no unresolved conflicts. - Verified the patches for `olap_scan_operator.cpp` and `like_column_predicate.h` match the original commit. - Verified `test_ngram_bloomfilter_index_like_escape.groovy` is identical to the original commit. - Confirmed branch-4.1 already has `LikeState::has_custom_escape`. - Compilation and runtime regression tests were not run in this session; CI validation remains pending. ### Release note Fix incorrect LIKE query results with NGRAM Bloom Filter indexes when the pattern contains an escape. ### Check List (For Author) - Test - [x] Regression test included from #68021 (not executed in this session) - [ ] Unit Test - [ ] Manual test - Behavior changed: - [x] Yes. Skip NGRAM Bloom Filter pruning for LIKE patterns that can carry an escape. - Does this need documentation? - [x] No. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label Co-authored-by: Chenyang Sun <sunchenyang@selectdb.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)