github-actions[bot] commented on code in PR #66788:
URL: https://github.com/apache/doris/pull/66788#discussion_r3785419760
##########
be/src/exprs/function/like.cpp:
##########
@@ -36,6 +37,109 @@
#include "exprs/function/simple_function_factory.h"
namespace doris {
+namespace {
+
+bool is_larger_than_fifty(std::string_view str) {
+ int number = 0;
+ auto [_, error] = std::from_chars(str.data(), str.data() + str.size(),
number);
+ return error == std::errc() && number > 50;
+}
+
+std::string mask_escaped_characters_and_character_classes(std::string_view
regexp) {
+ std::string masked_regexp(regexp);
+ bool escaped = false;
+ bool in_character_class = false;
+ bool character_class_can_close = false;
+ for (char& masked_character : masked_regexp) {
+ const char current = masked_character;
+ if (escaped) {
+ masked_character = ' ';
+ escaped = false;
+ if (in_character_class) {
+ character_class_can_close = true;
+ }
+ continue;
+ }
+ if (current == '\\') {
+ masked_character = ' ';
+ escaped = true;
+ continue;
+ }
+ if (in_character_class) {
+ masked_character = ' ';
+ if (current == ']' && character_class_can_close) {
+ in_character_class = false;
+ } else if (current != '^' || character_class_can_close) {
+ character_class_can_close = true;
+ }
+ continue;
+ }
+ if (current == '[') {
+ masked_character = ' ';
+ in_character_class = true;
+ character_class_can_close = false;
+ }
+ }
+ return masked_regexp;
+}
+
+/// Bounded repetitions can expand Hyperscan's compiler graph and make
compilation extremely
+/// expensive. This checker is adapted from ClickHouse's
`SlowWithHyperscanChecker`.
+class SlowWithHyperscanChecker {
+public:
+ SlowWithHyperscanChecker()
+ : _searcher_one_repeat(R"(\{\s*([\d]+)\s*,?\s*})"),
+ _searcher_two_repeats(R"(\{\s*([\d]+)\s*,\s*([\d]+)\s*\})") {}
+
+ bool is_slow(std::string_view regexp) const {
+ const std::string masked_regexp =
mask_escaped_characters_and_character_classes(regexp);
Review Comment:
[P1] Restrict this check to actual Hyperscan repeats
The lexical mask still leaves braces from valid non-repeat syntax visible.
For example, [Hyperscan supports `(?#
comment)`](https://intel.github.io/hyperscan/dev-reference/compilation.html#pattern-support),
so `(?# note{51})a` previously compiled, but this code sees `{51}`, skips
Hyperscan, and the default RE2 fallback fails because [RE2 does not support
that comment form](https://github.com/google/re2/blob/main/doc/syntax.txt).
Braced escapes such as `\x{51}`, quoted literals, POSIX classes, and
whitespace-bearing literal braces are misclassified similarly. Please follow
Hyperscan's lexical contexts and add default/strict tests for supported
non-repeat syntax.
##########
be/src/exprs/function/like.cpp:
##########
@@ -487,8 +596,21 @@ Status FunctionLikeBase::regexp_fn(const LikeSearchState*
state, const ColumnStr
}
// hyperscan compile expression to database and allocate scratch space
+bool FunctionLikeBase::should_fallback_to_re2(std::string_view regexp) {
+ static const SlowWithHyperscanChecker slow_with_hyperscan_checker;
+ return slow_with_hyperscan_checker.is_slow(regexp);
+}
+
Status FunctionLikeBase::hs_prepare(FunctionContext* context, const char*
expression,
hs_database_t** database, hs_scratch_t**
scratch) {
+ if (should_fallback_to_re2(expression)) {
Review Comment:
[P1] Do not route Hyperscan patterns to an incapable fallback
A real pattern such as `a{1001}` is supported by Hyperscan, but RE2 rejects
counting forms above 1000; even `\h{51}` stays under that limit yet uses a
Hyperscan-supported atom RE2 does not understand. This early return therefore
sends valid patterns to an engine that cannot represent them. Constant-open
errors with the default `enable_extended_regex=false` (Boost may recover only
when separately enabled), and the execute-time `ColumnConst` path has no Boost
fallback. Preserve a safe engine/error contract for every intercepted Hyperscan
pattern and test both the count ceiling and Hyperscan-only atoms.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -5594,6 +5600,7 @@ public TQueryOptions toThrift() {
tResult.setAnnIndexCandidateRowsPercentThreshold(annIndexCandidateRowsPercentThreshold);
tResult.setMergeReadSliceSize(mergeReadSliceSizeBytes);
tResult.setEnableExtendedRegex(enableExtendedRegex);
+ tResult.setEnableHyperscanFallback(enableHyperscanFallback);
Review Comment:
[P2] Forward this policy to BE constant folding
Normal execution gets the value here, but `FoldConstantRuleOnBE.evalOnBE()`
constructs a fresh `TQueryOptions` and never copies `enableHyperscanFallback`.
With `enable_fold_constant_by_be=true` and this setting false, a constant
expression such as `'a' REGEXP 'a{51}'` is evaluated with the thrift default
true and folded to a boolean instead of returning the requested strict error.
Populate the fold RPC from the current session (or explicitly set this field)
and cover this alternate sender in a test.
##########
be/src/exprs/function/like.cpp:
##########
@@ -36,6 +37,109 @@
#include "exprs/function/simple_function_factory.h"
namespace doris {
+namespace {
+
+bool is_larger_than_fifty(std::string_view str) {
+ int number = 0;
+ auto [_, error] = std::from_chars(str.data(), str.data() + str.size(),
number);
+ return error == std::errc() && number > 50;
+}
+
+std::string mask_escaped_characters_and_character_classes(std::string_view
regexp) {
+ std::string masked_regexp(regexp);
+ bool escaped = false;
+ bool in_character_class = false;
+ bool character_class_can_close = false;
+ for (char& masked_character : masked_regexp) {
+ const char current = masked_character;
+ if (escaped) {
+ masked_character = ' ';
+ escaped = false;
+ if (in_character_class) {
+ character_class_can_close = true;
+ }
+ continue;
+ }
+ if (current == '\\') {
+ masked_character = ' ';
+ escaped = true;
+ continue;
+ }
+ if (in_character_class) {
+ masked_character = ' ';
+ if (current == ']' && character_class_can_close) {
+ in_character_class = false;
+ } else if (current != '^' || character_class_can_close) {
Review Comment:
[P1] Treat the second leading caret as class content
In `[^^](ab?c?d){1000,5000}`, the first `^` negates the class but the second
is ordinary class content. This branch leaves `character_class_can_close` false
for both, so the real `]` does not close the class and the masker blanks the
subsequent genuine large repeat. `should_fallback_to_re2()` then returns false
and the expensive pattern still reaches `hs_compile()`. Track only the first
caret as special and add a regression for a large repeat following `[^^]`.
##########
gensrc/thrift/PaloInternalService.thrift:
##########
@@ -516,6 +516,8 @@ struct TQueryOptions {
229: optional i32 coordinator_thrift_max_message_size;
// FE can explicitly and idempotently acknowledge external-file commit
reports.
230: optional bool supports_external_file_report_ack = false;
+ // Fall back to RE2 when Hyperscan cannot compile a regular expression.
+ 231: optional bool enable_hyperscan_fallback = true;
Review Comment:
[P2] Preserve this option when ALTER jobs are replayed
Synchronous-MV jobs capture `SessionVariable.toThrift()`, but `AlterJobV2`
journals the nested `TQueryOptions` through metadata Gson. Its annotation-only
strategy drops generated Thrift fields, so an FE restart after the durable
PENDING record and before task dispatch reconstructs this option with the IDL
default `true`. A job created with fallback disabled and a WHERE predicate such
as `value_col REGEXP 'a{51}'` can therefore resume using RE2 instead of
returning the captured strict error. Persist the required execution options
through a Thrift-aware representation and cover replay before task dispatch.
##########
be/src/exprs/function/like.cpp:
##########
@@ -36,6 +37,109 @@
#include "exprs/function/simple_function_factory.h"
namespace doris {
+namespace {
+
+bool is_larger_than_fifty(std::string_view str) {
+ int number = 0;
+ auto [_, error] = std::from_chars(str.data(), str.data() + str.size(),
number);
+ return error == std::errc() && number > 50;
+}
+
+std::string mask_escaped_characters_and_character_classes(std::string_view
regexp) {
+ std::string masked_regexp(regexp);
+ bool escaped = false;
+ bool in_character_class = false;
+ bool character_class_can_close = false;
+ for (char& masked_character : masked_regexp) {
+ const char current = masked_character;
+ if (escaped) {
+ masked_character = ' ';
+ escaped = false;
+ if (in_character_class) {
+ character_class_can_close = true;
+ }
+ continue;
+ }
+ if (current == '\\') {
+ masked_character = ' ';
+ escaped = true;
+ continue;
+ }
+ if (in_character_class) {
+ masked_character = ' ';
+ if (current == ']' && character_class_can_close) {
+ in_character_class = false;
+ } else if (current != '^' || character_class_can_close) {
+ character_class_can_close = true;
+ }
+ continue;
+ }
+ if (current == '[') {
+ masked_character = ' ';
+ in_character_class = true;
+ character_class_can_close = false;
+ }
+ }
+ return masked_regexp;
+}
+
+/// Bounded repetitions can expand Hyperscan's compiler graph and make
compilation extremely
+/// expensive. This checker is adapted from ClickHouse's
`SlowWithHyperscanChecker`.
+class SlowWithHyperscanChecker {
+public:
+ SlowWithHyperscanChecker()
+ : _searcher_one_repeat(R"(\{\s*([\d]+)\s*,?\s*})"),
+ _searcher_two_repeats(R"(\{\s*([\d]+)\s*,\s*([\d]+)\s*\})") {}
+
+ bool is_slow(std::string_view regexp) const {
+ const std::string masked_regexp =
mask_escaped_characters_and_character_classes(regexp);
+ return is_slow_one_repeat(masked_regexp) ||
is_slow_two_repeats(masked_regexp);
Review Comment:
[P1] Account for multiplicative nested repeats
This checks each bound independently, so `(((ab?c?d?){50}){50}){50}` returns
false even though the nested repeats multiply four positions by `50^3` and
reach Hyperscan's 500,000-position expansion ceiling. [Hyperscan recursively
builds the repeated child before checking `vcount *
copies`](https://github.com/intel/hyperscan/blob/v5.4.2/src/parser/ComponentRepeat.cpp#L113-L152),
so adding another `{50}` layer still constructs that 500,000-position child
before the outer rejection. Track effective nested expansion (or conservatively
intercept nested bounded products) and add direct/open/execute regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -3513,6 +3515,10 @@ public void
checkAnnIndexCandidateRowsPercentThreshold(String value) {
description = "Enable extended regular expressions, support
look-around zero-width assertions")
public boolean enableExtendedRegex = false;
+ @VarAttrDef.VarAttr(name = ENABLE_HYPERSCAN_FALLBACK, needForward = true,
affectQueryResultInExecution = true,
Review Comment:
[P2] Invalidate cached point-query state when this value changes
Marking the variable execution-affecting does not refresh prepared
short-circuit contexts. `ShortCircuitQueryContext` serializes `TQueryOptions`
once, while `isReusable()` checks table metadata and only
`fileCacheQueryLimitBytes`; BE then keeps that RuntimeState and its opened
output expressions. A prepared point query with `value_col REGEXP 'a{51}'` can
therefore keep the RE2-backed state after the session changes this setting from
true to false. Include the execution-variable digest (or this field) in the
reuse fence and test a setting change between executions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
