[fix](be) Fall back from expensive Hyperscan bounded repeats - #66788
[fix](be) Fall back from expensive Hyperscan bounded repeats#66788HappenLee wants to merge 6 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Hyperscan compilation can become extremely expensive for regular expressions with large bounded repetitions, such as `prompt_rewrite\.h03.{0,1000}429`. Doris previously sent every compatible pattern to Hyperscan before considering RE2, so compiling such expressions could consume excessive CPU and delay query execution. Detect bounded repetitions above 50 before calling Hyperscan and reuse the existing RE2 fallback path for both constant and non-constant patterns. Keep the detector local to the LIKE/REGEXP implementation and cover its threshold behavior and end-to-end matching results.
### Release note
Fall back to RE2 for regular expressions whose bounded repetition exceeds 50 to avoid expensive Hyperscan compilation.
### Check List (For Author)
- Test: Unit Test
- `./run-be-ut.sh -j 48 --run --filter=FunctionLikeTest.hyperscan_bounded_repeat_fallback:FunctionLikeTest.hyperscan_bounded_repeat_threshold`
- Behavior changed: Yes. Large bounded repetitions use RE2 instead of Hyperscan while preserving REGEXP results.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Hyperscan compilation can fail or be intentionally intercepted for regular expressions with expensive bounded repetitions. Doris always fell back to RE2, so users could not choose strict failure behavior. Add the enable_hyperscan_fallback session variable, propagate it through TQueryOptions, and return the Hyperscan status when fallback is disabled. Mask escaped characters and character classes before bounded-repeat detection so literal braces are not intercepted.
### Release note
Add the enable_hyperscan_fallback session variable. It defaults to true; setting it to false returns an error instead of falling back to RE2 when Hyperscan compilation is unavailable.
### Check List (For Author)
- Test:
- Unit Test: FunctionLikeTest.* and org.apache.doris.qe.SessionVariablesTest
- Behavior changed: Yes. Hyperscan fallback can now be disabled per session; the default behavior is unchanged.
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Requesting changes: seven substantiated correctness and lifecycle gaps remain.
- Goal and proof: The patch aims to keep expensive bounded repeats away from Hyperscan while preserving normal REGEXP behavior and offering a session-controlled strict mode. It does not yet accomplish that end to end: valid Hyperscan patterns can regress, expensive patterns can bypass the detector, and the policy is lost or stale in alternate execution paths.
- Scope and clarity: The patch is focused, but the hand-written lexical recognizer is not precise enough for Hyperscan grammar and the cross-FE/BE session-policy integration is incomplete.
- Concurrency: Function-local checker initialization and const RE2 matching are thread-safe; no new lock-order or shared-mutation defect was found.
- Lifecycle: Hyperscan database/scratch cleanup is sound. Prepared point-query reuse retains old query options and once-opened REGEXP state, and ALTER replay reconstructs the new option with the wrong value.
- Configuration: The session variable is dynamic, forwarded, and execution-affecting on the ordinary path, but strict-mode changes are not honored by BE constant folding, reusable point queries, or replayed synchronous-MV jobs.
- Compatibility: The optional Thrift field and default
trueare safe for ordinary mixed-version decoding; old BEs necessarily cannot enforce the new setting. The persisted ALTER-job path is not replay-equivalent after failover. - Parallel paths: Constant open, execute-time
ColumnConst, genuinely row-varying RE2, LIKE conversion, storage pushdown, state cloning, BE folding, point-query reuse, and schema-change/MV execution were traced. The seven inline comments cover every substantiated divergence. - Conditional checks and errors: Numeric parsing, the flat
> 50comparisons, status propagation, and allocation cleanup are sound. The detector nevertheless misreads supported lexical contexts, mishandles[^^], misses multiplicative nesting, and can route valid patterns to an incapable fallback engine. - Test coverage: Added tests cover flat thresholds, basic escapes/classes, direct default/strict paths, LIKE brace escaping, and direct
SessionVariable.toThrift(). They do not cover supported non-repeat syntax,[^^], nested repeat products, Hyperscan-only fallback inputs, BE-fold options, prepared-context invalidation, or ALTER replay. - Test results: The modified expectations match the implementation, but the whitespace-bearing brace case encodes the same lexical false positive described inline. No builds or tests were run in this review environment, as required by the review bundle.
- Observability: No new metric or logging requirement was identified; existing status paths are sufficient once the behavior is corrected.
- Persistence and transactions: No transaction protocol changed. The synchronous-MV edit-log/replay path fails to preserve the captured execution option across restart/failover.
- Data writes: No direct write atomicity or visibility issue was found; the materialized-view build issue is an execution-policy replay mismatch.
- FE/BE variable passing: Ordinary
SessionVariable.toThrift(), forwarding, cloning, and mixed-version defaults are sound. Three distinct alternate sender/cache/persistence boundaries remain incorrect. - Performance: The linear precheck itself is reasonable, but
[^^]and nested{50}products can still expose Hyperscan to the expensive compilation the patch is intended to prevent. - Additional focus: No extra user-provided review focus was present. No independent nullable, cloud/shared-nothing, MoW, or storage-format issue applies here.
Review completeness: the third and final allowed round produced a new valuable finding. This review is therefore capped/incomplete rather than converged; all seven currently substantiated findings are included.
### What problem does this PR solve? Issue Number: None Related PR: apache#66788 Problem Summary: The Hyperscan bounded-repeat masker treated every leading caret in a character class as the negation marker. For `[^^]`, the second caret is class content, but Doris kept the class open and masked the real bounded repeat that followed it. Track whether the one optional leading negation marker is still allowed so the closing bracket is recognized and the expensive repeat is intercepted. ### Release note Correct expensive Hyperscan repeat detection after negated character classes. ### Check List (For Author) - Test: Unit Test - `GLIBC_COMPATIBILITY=OFF ./run-be-ut.sh -j 48 --run --filter=FunctionLikeTest.*` - Behavior changed: Yes. Large bounded repeats following character classes such as `[^^]` are now intercepted instead of reaching Hyperscan. - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Requesting changes: two distinct gaps remain on head 17e768572e5cc2d7bd95771f988b63e2dc04c99d.
- P1 — parallel public Hyperscan compilers bypass the guard. The new checker is private to
FunctionLikeBase::hs_prepare(). No-index/indexedMATCH_REGEXP, the indexedsearch()DSLREGEXPleaf, andmulti_match_anystill pass the same expensive repeat directly tohs_compile()/hs_compile_multi(). - P2 — load planners lose the new execution policy. Broker, routine, and ordinary stream-load paths construct fresh
TQueryOptionsand retain field 231's defaulttrue; broker/routine jobs also do not persist the creator's value. A load regexp therefore falls back instead of returning the requested strict error, including when a new stream-load context inherited a persisted globalfalse.
Critical-checkpoint conclusions:
- Goal and proof: the changed constant-pattern REGEXP/LIKE paths do intercept direct bounds above 50 and the added BE tests cover default fallback, strict open/execute, threshold, escapes, classes, and the
[^^]fix. The end-to-end goal is incomplete because the public compiler paths above remain exposed, and the FE test proves only directSessionVariable.toThrift()propagation. - Scope, clarity, and reuse: the local change is small and understandable, but placing the checker inside the LIKE implementation prevents the required reuse at other Hyperscan boundaries.
- Concurrency and lifecycle: the function-local checker has thread-safe initialization and const matching; THREAD_LOCAL function state, predicate clones, databases, and scratch allocations remain independently owned. New error exits propagate status and release/null owned resources. No new lock-order, race, static-initialization, nullable/const-shape, or cleanup defect was found. The material lifecycle omission is the uncaptured/fresh load state in P2; cached point-query and ALTER replay issues are already covered by live threads.
- Dynamic variable propagation: normal legacy/Nereids coordination, forwarded execution, fragment copies, and execution-result hashing preserve the value. Fresh load senders do not, even for the persistent global setting. BE constant folding is already covered by an existing thread.
- Compatibility: Thrift field 231 is unique, optional, and defaults to
true, preserving old-FE/new-BE behavior in Doris's documented BE-before-FE upgrade order. No storage-format or function-symbol incompatibility was introduced. - Parallel paths and conditions: repository-wide enumeration found four direct
hs_compile()sites plus the multi-pattern compiler template; only the LIKE/REGEXP site is guarded. Class/comment lexical approximations, nested multiplication, and fallback-engine capability are already covered by existing discussions, and the reported[^^]false negative is fixed on this head. - Tests and observability: no regression or end-to-end load/MATCH/search/multi test covers the two remaining gaps, and no result file changed. Existing statuses identify an interception or compile failure when the option arrives, but a dropped load option is silent. Per the review-only constraint, I did not run builds or tests; the PR reports 36 BE and 17 FE unit tests, while formatting/checkstyle and other lightweight checks are green and macOS BE UT is still pending at review time.
- Persistence, writes, and other invariants: no transaction visibility, version, delete-bitmap, or metadata-write mechanism changed. P2 can nevertheless allow a load with a regexp filter to proceed where strict mode requested failure, so the policy must be captured before dispatch and replay. The checker adds only linear pre-compilation work and no per-row regression was found.
There was no additional user-provided focus. In the second complete normal/risk review round, all reviewers returned NO_NEW_VALUABLE_FINDINGS; the two inline findings below are the final deduplicated set.
### What problem does this PR solve? Issue Number: None Related PR: apache#66788 Problem Summary: The expensive bounded-repeat check was local to LIKE/REGEXP, so MATCH_REGEXP, inverted-index regexp queries, and multi-match could still compile pathological expressions directly with Hyperscan. Load planners also created fresh query options and lost the enable_hyperscan_fallback value selected when broker, routine, or stream load work was created. Move the checker into a shared utility, reject expensive patterns on Hyperscan-only paths, and persist and propagate the fallback option through load jobs, task descriptors, and both legacy and Nereids coordinators. Old replayed jobs without the persisted value retain the default enabled behavior. ### Release note Apply bounded-repeat safeguards to all Hyperscan compilation paths and preserve enable_hyperscan_fallback for load execution. ### Check List (For Author) - Test: Unit Test - BE: 40 targeted FunctionLike, MATCH_REGEXP, multi-match, and inverted-index regexp tests - FE: 32 targeted load task, coordinator, and routine-load replay tests - Behavior changed: Yes. Hyperscan-only paths reject expensive bounded repeats, while load execution now honors the fallback setting captured when the work was created. - Does this need documentation: No
|
/review |
|
PR approved by at least one committer and no changes requested. |
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#66788 Problem Summary: The load fallback propagation change placed SessionVariable imports out of lexicographical order and omitted the required blank line between two test methods, causing FE checkstyle to fail. Restore the expected import ordering and method separation. ### Release note None ### Check List (For Author) - Test: No need to test (code-style-only change; per request, compilation and tests were not rerun) - Behavior changed: No - Does this need documentation: No
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Requesting changes: three distinct P2 execution-policy gaps remain on head ed8c4c190c1cd61508bdc1d27766e5187feb52e2.
- A server-prepared group-commit insert reuses fragment bytes containing the first execution's
enable_hyperscan_fallbackvalue after the session setting changes. - RPC stream-load planning bypasses its locked request snapshot and reads the mutable global default without
VariableMgr's protecting lock, racing with a concurrent global update. - During Doris's supported non-master-FE-first/master-last rolling upgrade, an upgraded follower forwards the new session key to an old master that silently drops it; the already-new BEs then restore the historical default.
Critical-checkpoint conclusions:
- Goal and proof: The PR aims to keep expensive bounded repeats away from Hyperscan and make fallback behavior dynamically controllable. The shared BE guard now covers all six direct compiler calls, and the normal current-version query/load paths propagate the option, but the three inline lifecycle/publication/compatibility gaps prevent end-to-end correctness.
- Scope and user focus: The implementation remains focused despite the necessary load-path propagation. All 45 authoritative changed files were reviewed. The user supplied no additional focus, so the full PR was reviewed without extra narrowing.
- Concurrency:
MF-2is the one new shared-memory defect: MySQLSET GLOBALwrites the ordinary field underVariableMgr.wlock, while a FrontendService RPC thread reads it unlocked after establishing a read-locked request snapshot. Other new task fields are creator-before-dispatch copies; no nested lock order, deadlock, or heavy-under-lock issue was found. - Lifecycle and static initialization: Function-local immutable checker initialization is thread-safe,
LikeSearchState::clone()carries the option, and Hyperscan database/scratch ownership is unchanged.MF-1is the remaining non-intuitive lifecycle: serialized prepared group-commit state outlives the session value that created it. - Dynamic configuration: Same-version normal execution observes session changes through
SessionVariable.toThrift(), forwarding metadata, and execution-result hashing. Prepared group commit (MF-1), concurrent RPC global capture (MF-2), and mixed-FE forwarding (MF-4) do not honor that contract. - Compatibility: Old-FE/new-BE decoding is safe because optional field 231 defaults to historical
true; new-FE/old-BE dispatch is outside Doris's documented BE-first upgrade order. The supported upgraded-follower/old-master interval is not safe because the old receiver drops the unknown forwarded map key (MF-4). - Parallel paths and conditions: LIKE/REGEXP, no-index MATCH, inverted-index v1, search-DSL v2, and ordinary/edit-distance multi-pattern compilers are all guarded before compilation/allocation. Legacy/Nereids query coordination, broker/cloud broker, Kafka/Kinesis routine load, ordinary/cloud RPC stream, insert-stream, initial group commit, and multi-table copying were traced. Lexical masking, nested repeats, incapable RE2 fallback, point-query reuse, BE constant folding, and ALTER replay are already covered by existing live threads and were not duplicated.
- Errors and cleanup: New guard failures reach established
Status/exception conversion boundaries; database and scratch cleanup remain intact. No ignored status, speculative recovery, nullable-shape, ownership, or static-initialization issue was found. - Test coverage: Added tests exercise each compiler family and selected option serialization/load/default paths. Missing behavior tests map directly to the three findings: prepared group-commit invalidation, latch/barrier-controlled global capture, and new-follower/old-master forwarding.
- Test results: This review runner forbids local builds/tests, so none were run here. At submission time, macOS BE UT, CheckStyle, Clang Formatter, license, dependency, and secret checks pass; TeamCity compile and regression contexts report failure. Their logs require authentication, so this review does not attribute those failures to a cause.
- Observability: Existing query/load identifiers and error boundaries are adequate once the policy is preserved; no new metric or logging requirement was identified.
- Persistence and transactions: Broker/routine job maps persist creator-time values and old records default to historical
true. No new EditLog, failover, transaction visibility, version, or delete-bitmap defect was found; the ALTER replay concern remains an existing live thread. - Data writes: Insert/load paths are affected only through whether regexp evaluation falls back or errors. No separate commit atomicity, row visibility, storage-format, MoW, or crash-cleanup issue was found.
- FE-BE variable passing: Every current-version
TQueryOptionsconstruction/copy and specialized load sender was inventoried. The remaining distinct boundaries are the three inline findings plus the already-threaded constant-fold, point-query, and ALTER paths. - Performance and other concerns: The checker adds linear masking/search work before compilation and outside locks; scalar option copies and map lookups are negligible. No additional CPU, allocation, cloud/shared-nothing, nullability, or storage concern survived the final sweep.
Review completeness: converged. In the third and final round, the normal BE, normal FE, and risk-focused reviewers all returned NO_NEW_VALUABLE_FINDINGS; all accepted, dismissed, and duplicate candidates have explicit conclusions.
| .setStrictMode(ConnectContext.get().getSessionVariable().enableInsertStrict); | ||
| NereidsStreamLoadTask streamLoadTask = NereidsStreamLoadTask.fromTStreamLoadPutRequest(streamLoadPutRequest); | ||
| NereidsStreamLoadTask streamLoadTask = NereidsStreamLoadTask.fromTStreamLoadPutRequest( | ||
| streamLoadPutRequest, ConnectContext.get().getSessionVariable().enableHyperscanFallback); |
There was a problem hiding this comment.
[P2] Invalidate prepared group-commit plans for this option
This value is serialized into execPlanFragmentParamsBytes when the planner is created, but executeGroupCommitInsert() reuses that planner whenever only the table id and base schema version match. After a session executes a prepared group-commit insert, changes enable_hyperscan_fallback, and executes it again, BE-evaluated expressions (for example a generated column using REGEXP 'a{51}') still receive the old policy. Store this option or the execution-variable digest on the planner and include it in the reuse fence, with a prepared group-commit regression that changes the setting between executions. This is distinct from the existing point-query thread because it is the separate PreparedStatementContext.groupCommitPlanner cache.
There was a problem hiding this comment.
Thanks for pointing this out. We agree that prepared Group Commit planner reuse should be invalidated through a unified session-variable fingerprint, instead of adding another option-specific reuse check here. Since that requires a broader change to the common cache invalidation mechanism, we will address it in a follow-up PR and track it with a Jira TODO, rather than expanding the scope of this PR. We will keep this thread open until the follow-up is tracked.
### What problem does this PR solve? Issue Number: None Related PR: apache#66788 Problem Summary: RPC stream load planning read enable_hyperscan_fallback directly from the mutable global default session variable without holding VariableMgr's read lock. Use the request-scoped ConnectContext session snapshot instead so planning observes a consistent value, and add a unit test that distinguishes the snapshot value from the global default. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.load.StreamLoadHandlerTest - Behavior changed: No - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Requesting changes for one additional, distinct P2 execution-policy gap on head 68f72081dccb35377e5f4793c5f7841425b88e74.
Asynchronous MTMV refreshes persist and restore only plan-affecting session variables. Because enable_hyperscan_fallback is marked only as execution-affecting, a creator's strict false value is lost before the first background refresh and after replay; the refresh uses the fresh context's default true and falls back instead of returning the requested error. The inline comment contains the full trace and requested coverage.
Critical-checkpoint conclusions:
- Goal and proof: the head guards all six direct Hyperscan compiler sites and threads the option through ordinary query and load execution. The MTMV context trace demonstrates one remaining reachable loss of the strict policy.
- Scope and clarity: the helper, Thrift field, FE propagation, load snapshots, and tests are otherwise internally consistent; no unrelated implementation change was introduced in the reviewed patch.
- Concurrency and lifecycle: the previously reported RPC stream-load snapshot race is fixed on this head. Broker, routine, SQL, transaction, cloud/local, legacy/Nereids, and multi-table task lifecycles preserve their captured value; MTMV's disconnected refresh context does not.
- Static/global state and configuration dynamics: old load journals default compatibly to
true, and request/job snapshots avoid mutable-global reads. Dynamic point-query/group-commit reuse and mixed-FE forwarding concerns already have live threads and are not duplicated here. - Compatibility: the optional Thrift field and missing old-record keys retain the historical
truedefault. Existing mixed-version concerns remain fenced by their current discussion; the new finding is an independent same-version MTMV persistence gap. - Parallel paths and error handling: constant/dynamic LIKE/REGEXP, no-index MATCH, v1/v2 index, multi-pattern/edit-distance, coordinator variants, and load planners were traced. Strict interception reports an error where the initiating policy reaches BE; MTMV incorrectly changes that condition by sending
true. - Tests and results: changed BE/FE tests cover the ordinary threshold, strict/default, compiler, load, and replay paths, but not initial or replayed MTMV refresh. No local build or test was run because the review runner prohibits it. Current CI has CheckStyle, Clang Formatter, dependency review, license, secrets, and large-file checks passing; macOS BE UT remains pending.
- Observability, persistence, and data writes: no separate observability defect was found. MTMV's durable plan-only session map is the persistence boundary that loses the value, and a regexp filter can therefore materialize rows under fallback semantics instead of failing strictly.
- FE/BE interface and performance: normal coordinators serialize the option correctly, but MTMV supplies the wrong fresh-context value before that boundary. All direct compiler sites invoke the cost guard before compilation; no distinct unthreaded compiler or additional performance defect was found beyond issues already discussed.
- Additional focus: no extra user-provided review focus was supplied.
The current head was checked against the authoritative 46-file bundle. Two full rounds converged: the second normal BE, normal FE, and separate risk-focused reviews all returned no new valuable findings, and the final comment set remained unchanged.
|
run buildall |
TPC-H: Total hot run time: 17770 ms |
TPC-DS: Total hot run time: 85511 ms |
ClickBench: Total hot run time: 14.65 s |
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Hyperscan can spend excessive time and memory compiling bounded repetitions with large limits. Doris: regex quantifiers above 50 are detected before Hyperscan compilation and processed by RE2 instead.
The new
enable_hyperscan_fallbacksession variable controls strictness. It defaults totrue, preserving fallback behavior. When set tofalse, Doris returns the Hyperscan/interception error instead of falling back. Escaped braces and braces inside character classes are masked before detection so regex literals are not misclassified.Release note
Add
enable_hyperscan_fallback. It defaults totrue; setting it tofalsereturns an error when Hyperscan cannot compile or safely process the regular expression.Check List (For Author)
GLIBC_COMPATIBILITY=OFF ./run-be-ut.sh -j 48 --run --filter=FunctionLikeTest.*(36 passed)DORIS_THIRDPARTY=$PWD/thirdparty ./run-fe-ut.sh --run org.apache.doris.qe.SessionVariablesTest(17 passed)