github-actions[bot] commented on code in PR #66050:
URL: https://github.com/apache/doris/pull/66050#discussion_r3681145562
##########
be/src/exprs/function/function_regexp.cpp:
##########
@@ -779,38 +831,67 @@ struct RegexpExtractAllImpl {
if (!st) {
context->add_warning(error_str.c_str());
handler.push_null(index_now, null_map);
- return;
+ return Status::OK();
}
engine = scoped_engine.get();
}
- if (engine->number_of_capturing_groups() == 0) {
- handler.push_empty(index_now);
- return;
+ // Same contract as Spark: the group index must be within
+ // [0, number_of_capturing_groups], anything else is an error.
+ const int num_groups = engine->number_of_capturing_groups();
+ if (index_data < 0 || index_data > num_groups) {
Review Comment:
The new bounds check has two call-path regressions. First, the two-argument
overload reaches it with implicit `idx = 1`, so a zero-capture regex now
errors; before this PR it called `push_empty`, and existing
`regexp_extract_all('xxfs','f')` / array regressions still expect empty
results. Second, default null handling executes nested values for mixed
nullable blocks before restoring null maps, so `(NULL,'(a)',2),
('abc','(a)',1)` aborts on the NULL row instead of returning NULL there. Please
preserve legacy no-index/no-capture behavior and skip nullable rows before
explicit `idx` validation; NULL-literal tests are FE-folded and do not cover
this BE path.
##########
regression-test/suites/query_p0/sql_functions/string_functions/test_string_function_regexp.groovy:
##########
@@ -255,4 +255,39 @@ suite("test_string_function_regexp") {
qt_sql_field4 "SELECT FIELD('21','2130', '2131', '21');"
qt_sql_field5 "SELECT FIELD(21, 2130, 21, 2131);"
+
+ qt_sql_regexp_extract_all_group0 "select
regexp_extract_all('x=a3&x=18abc&x=2&y=3&x=4&x=17bcd', 'x=([0-9]+)([a-z]+)',
0);"
+ qt_sql_regexp_extract_all_group2 "select
regexp_extract_all('x=a3&x=18abc&x=2&y=3&x=4&x=17bcd', 'x=([0-9]+)([a-z]+)',
2);"
+ qt_sql_regexp_extract_all_empty_group "select regexp_extract_all('a b',
'(a)|(b)', 2);"
+ qt_sql_regexp_extract_all_array_group0 "select
regexp_extract_all_array('x=a3&x=18abc&x=2&y=3&x=4&x=17bcd',
'x=([0-9]+)([a-z]+)', 0);"
+ qt_sql_regexp_extract_all_array_group2 "select
regexp_extract_all_array('x=a3&x=18abc&x=2&y=3&x=4&x=17bcd',
'x=([0-9]+)([a-z]+)', 2);"
+ qt_sql_regexp_extract_all_array_group3 "select
regexp_extract_all_array('hitdecisiondlist', '(i)(.*?)(e)', 3);"
+
+ // column input
+ qt_sql_regexp_extract_all_col_group0 "SELECT regexp_extract_all(k,
'(ll)(i)', 0) from test_string_function_regexp ORDER BY k;"
Review Comment:
These column cases run after `test_string_function_regexp` is dropped at
line 193 and after the suite switches to `test_query_db`; the table is never
recreated. The first query therefore fails with a missing-table error instead
of producing the added `.out` rows or exercising the three-argument path.
Please move these cases before the drop (or recreate/populate the table here)
and regenerate the expected output with the regression runner.
##########
be/src/exprs/function/function_regexp.cpp:
##########
@@ -159,9 +165,14 @@ struct RegexpExtractEngine {
pos += 1;
continue;
}
- // Extract first capturing group
- if (matches.size() > 1 && !matches[1].empty()) {
- results.emplace_back(matches[1].data(), matches[1].size());
+ // Extract the capturing group with the given index
+ if (static_cast<size_t>(index) < matches.size()) {
Review Comment:
For the newly supported `idx = 0`, this drops valid zero-length matches
before the selected group is appended, and `while (pos < size)` also skips a
terminal match. For example, `regexp_extract_all_array('b', 'a*', 0)` returns
`[]` here, while Spark's matcher yields `["", ""]` (offsets 0 and 1); empty
input/pattern should similarly yield one empty match. Please emit the selected
group for successful empty matches and advance safely while allowing one
terminal-position search.
##########
be/src/exprs/function/function_regexp.cpp:
##########
@@ -174,8 +185,8 @@ struct RegexpExtractEngine {
boost::match_results<const char*> matches;
while (boost::regex_search(search_start, search_end, matches,
*boost_regex)) {
- if (matches.size() > 1 && matches[1].matched) {
- results.emplace_back(matches[1].str());
+ if (static_cast<size_t>(index) < matches.size()) {
Review Comment:
For a zero-width match found after the current origin, `search_start += 1`
advances from the old origin, not from `matches[0].first`. Thus
`regexp_extract_all_array('ab', '(?=b)', 0)` (Boost path) emits the same
lookahead twice; multibyte input can also move the next search into UTF-8
continuation bytes. Please base progress on the actual match location and use
an encoding-safe one-character advance after an empty match.
##########
be/test/exprs/function/function_like_test.cpp:
##########
@@ -399,6 +465,61 @@ TEST(FunctionLikeTest, regexp_extract_all_array) {
}
}
+TEST(FunctionLikeTest, regexp_extract_all_invalid_group_index) {
+ // Same contract as Spark: a group index outside [0,
number_of_capturing_groups]
+ // is an error, for both regexp_extract_all and regexp_extract_all_array.
+ auto str_type = std::make_shared<DataTypeString>();
+ auto bigint_type = std::make_shared<DataTypeInt64>();
+
+ auto run_error_case = [&](const std::string& func_name, DataTypePtr
return_type, int64_t idx) {
+ auto col_str = ColumnString::create();
+ col_str->insert_data("hitdecisiondlist", 16);
+ auto col_pattern = ColumnString::create();
+ col_pattern->insert_data("(i)(.*?)(e)", 10);
Review Comment:
This literal is 11 bytes, so length 10 stores the malformed pattern
`"(i)(.*?)(e"`. Because the pattern is constant, `open(THREAD_LOCAL)` fails
before `execute()` can test either invalid index. Please use the actual string
size (or 11) so these cases validate the intended `idx` error path.
##########
be/src/exprs/function/function_regexp.cpp:
##########
@@ -159,9 +165,14 @@ struct RegexpExtractEngine {
pos += 1;
continue;
}
- // Extract first capturing group
- if (matches.size() > 1 && !matches[1].empty()) {
- results.emplace_back(matches[1].data(), matches[1].size());
+ // Extract the capturing group with the given index
+ if (static_cast<size_t>(index) < matches.size()) {
+ const re2::StringPiece& group = matches[index];
+ if (group.data() != nullptr) {
+ results.emplace_back(group.data(), group.size());
+ } else {
+ results.emplace_back();
+ }
}
// Move position forward
auto offset = std::string(str_pos, str_size)
Review Comment:
Advancing via a fresh textual `find` can select an earlier identical
substring that did not satisfy the regex context. With
`regexp_extract_all_array('ab b', '\\bb', 0)`, RE2 matches only the final `b`,
but `.find("b")` chooses the earlier non-boundary byte, so the loop reaches and
emits the final match twice. Please advance from the actual RE2 match pointer
(`matches[0].data() - str_pos`) plus its length.
--
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]
