Repository navigation
[fix](like) Fix LIKE matching values with extra characters at the end - #68133
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes. The exact-end \z conversion and the newline/literal-star cases are sound for valid UTF-8, but the new byte-semantics guarantee and a supported escape form remain incomplete.
Critical checkpoint conclusions:
- Goal: The patch fixes whole-value anchoring for newline tails and avoids forcing a terminal
%to traverse the remaining value, but it does not yet provide consistent invalid-byte behavior for every%position. - Scope/focus: The production delta is small and focused. No additional user review focus was provided.
- Concurrency: No new concurrency is introduced; LIKE function state remains thread-local and pushed-predicate clones own separate Hyperscan scratch state.
- Lifecycle/static initialization: No new special lifecycle, cross-TU static initialization, or ownership cycle is introduced.
- Configuration: No configuration item is added or changed.
- Compatibility: No function symbol, storage format, wire protocol, or rolling-upgrade contract changes.
- Parallel paths: I traced constant Hyperscan, nonconstant RE2, scalar/vector, cloned scan pushdown, NOT LIKE, dictionary/plain-column, and JSON-search paths. The invalid-byte engine split and explicit-backslash escape gap are called out inline; no separate clone or negation defect was found.
- Special conditions: Empty patterns, leading/interior/trailing and repeated
%,_, newline, literal regex metacharacters, and default/custom escapes were checked. The terminal branch is correct for valid UTF-8, subject to the two completeness gaps inline. - Tests: The unit test covers constant/nonconstant argument combinations, and the regression suite covers direct evaluation, pushdown, clone reconstruction, and NOT LIKE. The invalid-byte row relies on an undefined Hyperscan contract, while interior-invalid-byte and explicit-backslash-escape cases are missing. This was a static-only review; the task contract prohibited builds or test execution.
- Expected results: All 74 added result lines are deterministic and consistent with the inserted rows.
- Observability: No new observability surface is needed for this local matcher change.
- Transactions/persistence/data writes: Not applicable; no metadata, EditLog, transaction, or storage write path changes.
- FE/BE variable propagation: Not applicable; no new cross-layer variable is introduced.
- Performance: Removing the terminal
.*avoids unnecessary suffix traversal; no additional CPU or memory regression was found. - Other issues: No other correctness, null-handling, error-propagation, compatibility, or performance issue remained after the second converged review round.
| // A pattern open at the tail must not make the engines walk the rest of the value: | ||
| // `.*` only matches valid UTF-8, so anchoring it with `\z` would make RE2 reject a | ||
| // value whose tail is not valid UTF-8 while Hyperscan still accepts it. | ||
| {{std::string("acb\xff", 4), std::string("a_b%")}, uint8_t(1)}, |
There was a problem hiding this comment.
[P2] Avoid asserting undefined Hyperscan UTF-8 behavior
check_function_all_arg_comb includes constant-pattern runs, and a_b% bypasses the LIKE fast paths because of _. That makes construct_like_const_state compile ^a.b with HS_FLAG_UTF8 and pass this four-byte acb\xff buffer to hs_scan. Hyperscan's API explicitly says that scanning invalid UTF-8 with this flag has undefined results, so this expected 1 can vary across engine versions or architectures and cannot prove Hyperscan/RE2 equivalence. Please route invalid input through a matcher with defined byte semantics (or otherwise keep it out of the UTF-8 Hyperscan path) before asserting this result.
| // a trailing `%` matches anything, and so does appending nothing | ||
| return; | ||
| } | ||
| re_pattern->append(".*"); |
There was a problem hiding this comment.
[P2] Handle invalid bytes for interior percent wildcards too
The new invalid-input case establishes that SQL % must be able to absorb bytes that are not valid UTF-8, but this change makes only the terminal wildcard byte-safe. For example, a%b still becomes ^a.*b\z; the nonconstant path runs that through UTF-8 RE2, where . matches a code point and cannot traverse the 0xff in bytes [0x61, 0xff, 0x62], so it returns false. The constant-pattern Hyperscan path is undefined for the same invalid input, creating an additional path split. Although the interior case predates this hunk, it leaves the byte-semantics contract introduced here incomplete. Please give every % defined byte-safe semantics (or validate/route invalid input consistently), and cover constant, nonconstant, pushdown, and NOT LIKE forms with an invalid byte between required literals. RE2 syntax reference
| } | ||
|
|
||
| if (c == '%') { | ||
| if (i + 1 == pattern.size()) { |
There was a problem hiding this comment.
[P2] Preserve wildcard parity with an explicit backslash escape
This leaves a supported trailing-wildcard form outside the fix. With a one-byte backslash as the explicit escape, pattern bytes [0x5c, 0x5c, 0x25] mean a literal backslash followed by wildcard %, but replace_pattern_by_escape first collapses the pair to [0x5c, 0x25]. Downstream fast-path recognition can then classify % as a literal, and the regex fallback likewise consumes it as escaped, so the pattern no longer has an open literal-backslash prefix. The normalization bug predates this branch, but both constant and nonconstant three-argument LIKE paths remain incorrect. Please preserve the original default-backslash representation (or otherwise retain escape parity), and add coverage showing that a literal-backslash prefix followed by % remains open.
88cd134 to
93c7f5d
Compare
|
/review |
There was a problem hiding this comment.
Requesting changes. The \z exact-end anchor fixes terminal-newline and literal-* matching for valid UTF-8, but applying it after a genuine trailing % introduces a distinct full-tail performance regression. The added NOT LIKE row also does not cover the specialized opposite column-predicate path its surrounding pushdown comment implies.
Critical checkpoint conclusions:
- Goal: Whole-value matching for closed LIKE patterns is corrected, including empty values, terminal newlines, literal regex metacharacters, and escaped
%. A genuine terminal wildcard should remain open; M-1 identifies the resulting performance regression. - Scope/focus: The production change is small and focused. No additional user review focus was provided.
- Concurrency: No new concurrency is introduced. Function state remains thread-local, and each pushed predicate owns separate cloned Hyperscan scratch state.
- Lifecycle/static initialization: No new special lifecycle, ownership cycle, or cross-TU static initialization is introduced.
- Configuration: No configuration item is added or changed.
- Compatibility: No function symbol, storage format, wire protocol, or rolling-upgrade contract changes.
- Parallel paths: Constant Hyperscan, nonconstant/fallback RE2, scalar/vector fast paths, cloned storage pushdown, generic NOT LIKE expression pushdown, and
json_searchreuse were traced. M-1 affects regex paths with a real trailing%; M-2 records that the NOT LIKE query missesLikeColumnPredicate(opposite=true). - Special conditions: Empty patterns, leading/interior/trailing and repeated
%,_, embedded/terminal newlines, literal*, escaped%, default/custom escapes, and valid UTF-8 multibyte characters were checked. Existing invalid-byte and explicit-backslash concerns are already covered by prior inline threads and were not duplicated. - Tests: The BE matrix covers constant/nonconstant argument combinations, and the regression suite covers direct execution plus positive specialized pushdown. Checked-in outputs are deterministic and consistent with the fixture. The NOT LIKE row is generic expression coverage rather than specialized predicate-clone coverage. CI skipped the build/test job for this head, and the review contract prohibited local builds or test execution, so this conclusion is static-only.
- Observability: No new observability surface is needed for this local matcher change.
- Transactions/persistence/data writes: Not applicable; no metadata, EditLog, transaction, or storage write path changes.
- FE/BE variable propagation: Not applicable; no new cross-layer variable is introduced.
- Performance: M-1 changes successful regex-path trailing-wildcard matches from prefix-bounded callback termination to a full-value scan.
- Other issues: M-2 is a P3 test-description/coverage gap. No other distinct correctness, null-handling, error-propagation, compatibility, or performance issue remained after two converged review rounds.
| re_pattern->append("$"); | ||
| } | ||
| // `\z` is the end of the value in both Hyperscan and RE2 | ||
| re_pattern->append("\\z"); |
There was a problem hiding this comment.
[P2] Keep a genuine trailing wildcard open
Appending \z unconditionally makes a regex-requiring pattern such as a_b% compile as ^a.b.*\z. Doris's Hyperscan callback stops scanning at the first reported match, so the previous open-tail form can finish after the required prefix, while the new end anchor cannot report until it has traversed the entire value. On a long valid string beginning with axb, this turns a prefix-bounded match into an O(value length) scan; the scalar fallback and cloned pushdown state receive the same unnecessary constraint. Hyperscan documents that callbacks expose match end offsets and may halt a partially completed scan. Please omit the tail anchor when the final LIKE token is an unescaped % (while retaining \z for literal * and escaped %) and update the shape coverage accordingly.
There was a problem hiding this comment.
a_b% compile as ^a.b
| qt_pushdown_literal_star """ | ||
| select id from test_like_whole_string_match where s like 'a_*' order by id | ||
| """ | ||
| qt_pushdown_not_literal_star """ |
There was a problem hiding this comment.
[P3] Do not present this as specialized NOT LIKE pushdown coverage
Nereids sends this as CompoundPredicate(NOT, LIKE(...)). _normalize_predicate checks the NOT node's direct children first; child zero is the LIKE function rather than the slot, so it returns before _normalize_function_filters can build LikeColumnPredicate(opposite=true) and clone the matcher state. On this DUP_KEYS table the whole expression may still use generic expression pushdown, so the result is valid NOT LIKE behavior coverage, but it does not cover the specialized path implied by the surrounding comment. Please rename or move this case accordingly. If specialized NOT LIKE pushdown is enabled later, add focused path tests together with nullable handling—the current opposite predicate retains NULL rows and cannot safely replace the residual expression by itself.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
LIKE has to match the whole value, but both engines that evaluate the converted pattern search for a match anywhere in the value, so the pattern carries the anchors itself. The tail anchor was `$`, which Hyperscan reads the PCRE way: it also matches right before a newline that ends the value. A value of 'acb' followed by a line feed therefore matched LIKE 'a_b', and a filter on such a column returned extra rows. RE2, used when Hyperscan cannot compile the pattern, treats `$` as the end of the value, so the two engines disagreed on the same pattern. Use `\z`, which is the end of the value in both. The tail anchor was also dropped whenever the produced regex ended with '*'. That check is meant to detect the `.*` expanded from a trailing `%`, but an escaped literal `*` ends the regex with the same character, so LIKE 'a_*' lost its anchor and matched 'ab*xyz'. Decide it from the pattern instead: a `%` in the last position returns right away, which leaves the regex unanchored and skips the redundant `.*`, while every other pattern gets the anchor. Keeping the tail open also matters for RE2: anchoring a trailing `%` as `^a.b.*\z` makes `.*` run to the end of the value, which costs 1.9ms on a 1MB value against 55ns unanchored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
93c7f5d to
ad1cd80
Compare
|
/review |
There was a problem hiding this comment.
Automated review conclusion: no new valuable findings on ad1cd80f4e369cc95d77e941908fbfc16f6820d6.
Critical checkpoint conclusions:
- Goal and proof: the change replaces Hyperscan's newline-permissive
$tail with exact end-of-value\z, leaves a genuine terminal%open, and still anchors a literal trailing*or escaped%. The unit matrix, direct shape assertions, and ordered regression expectations cover the changed outcomes across constant, nonconstant, shortcut, and positive pushdown paths. - Scope and clarity: the implementation is focused on one converter plus its BE unit and regression coverage. No unrelated source changes are present.
- Error handling, memory, concurrency, and lifecycle: no new ignored
Status, tracked allocation, thread, lock, or global/static lifecycle is introduced. Function state remains thread-local, andLikeColumnPredicaterebuilds each matcher with independent Hyperscan scratch before concurrent scan use. - Configuration and compatibility: no configuration, FE/BE protocol, persisted metadata, storage format, function symbol, or rolling-upgrade behavior changes. Both Hyperscan and RE2 support
\z; the same converter and dot-newline semantics are used by constant Hyperscan, row-varying RE2, and cloned pushdown state. - Parallel paths and special conditions: escape processing runs before the terminal-
%return, so default escape parity and non-backslash custom escapes retain the intended literal/wildcard distinction. Fast paths agree with the regex path. The known explicit-backslash custom-escape normalization concern remains covered by the existing inline thread and is not duplicated here. - Test coverage and results:
check_function_all_arg_combexercises constant and nonconstant argument forms, the shape test checks emitted regexes, and the DUP_KEYS positive filters reachLikeColumnPredicatecloning. Regression queries are ordered and the.outrows are internally correct. The corrected NOT LIKE case accurately states that it remains generic rather than specialized pushdown. - Observability, transactions, data writes, and cross-process variables: not applicable to this local expression-matching correction; no new logging or metrics are needed.
- Performance and other risks: omitting
.*\zfor a real trailing%preserves prefix-bounded termination and avoids a whole-value scan. No additional hot-path copy or allocation was introduced. A bounded escape/token review found no distinct issue beyond the existing duplicate-fenced threads.
Verification status: static-only. The review contract prohibited builds and tests. Current GitHub checks show formatting/license-style success but do not provide functional-test proof for this head, so the added test results were reviewed rather than executed here.
User focus: no additional user-provided focus was supplied; the complete four-file diff was reviewed.
Opinion: no new blocking issue or inline comment is warranted beyond the existing review context.
|
run buildall |
TPC-H: Total hot run time: 28020 ms |
TPC-DS: Total hot run time: 153248 ms |
ClickBench: Total hot run time: 23.97 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…#68133) ### What problem does this PR solve? 1. `LIKE 'a_b'` is converted to `^a.b\z`, not `^a.b$`. Hyperscan follows PCRE, where `$` also matches right before a newline that ends the value, so `^a.b$` wrongly matched 'acb\n'. `\z` is the end of the value in both Hyperscan and RE2. 2. `LIKE 'a_b%'` is converted to `^a.b`, not `^a.b.*\z`. A trailing `%` matches anything, and so does appending nothing. The anchored form would force `.*` to consume the value to its end, and `.*` only matches valid UTF-8, so RE2 would not match 'acb\xff' while Hyperscan still would. 3. `LIKE 'a_*'` is converted to `^a.\*\z`, not `^a.\*`. The tail anchor used to be skipped whenever the produced regex ended with `*`, a check meant to detect the `.*` expanded from a trailing `%`. An escaped literal `*` ends the regex with the same character, so the pattern lost its anchor entirely and `'ab*xyz'` matched. The decision now comes from the position in the pattern instead of from the produced string. Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [x] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
What problem does this PR solve?
LIKE 'a_b'is converted to^a.b\z, not^a.b$. Hyperscan follows PCRE, where$also matches right before a newline that ends the value, so^a.b$wrongly matched 'acb\n'.\zis the end of the value in both Hyperscan and RE2.LIKE 'a_b%'is converted to^a.b, not^a.b.*\z. A trailing%matches anything, and so does appending nothing. The anchored form would force.*to consume the value to its end, and.*only matches valid UTF-8, so RE2 would not match 'acb\xff' while Hyperscan still would.LIKE 'a_*'is converted to^a.\*\z, not^a.\*. The tail anchor used to be skipped whenever the produced regex ended with*, a check meant to detect the.*expanded from a trailing%. An escaped literal*ends the regex with the same character, so the pattern lost its anchor entirely and'ab*xyz'matched. The decision now comes from the position in the pattern instead of from the produced string.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)