ccl125 commented on code in PR #66050:
URL: https://github.com/apache/doris/pull/66050#discussion_r3680675977
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/RegexpExtractAll.java:
##########
@@ -61,7 +74,7 @@ private RegexpExtractAll(ScalarFunctionParams functionParams)
{
*/
@Override
public RegexpExtractAll withChildren(List<Expression> children) {
- Preconditions.checkArgument(children.size() == 2);
+ Preconditions.checkArgument(children.size() == 2 || children.size() ==
3);
Review Comment:
Update: the design changed based on the rolling-upgrade feedback below — the
FE no longer pads a default index, so a node can legitimately have 2 or 3
children and the assertion stays `2 || 3`. The BE now accepts both arities.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/RegexpExtractAllArray.java:
##########
@@ -64,7 +77,7 @@ private RegexpExtractAllArray(ScalarFunctionParams
functionParams) {
*/
@Override
public RegexpExtractAllArray withChildren(List<Expression> children) {
- Preconditions.checkArgument(children.size() == 2);
+ Preconditions.checkArgument(children.size() == 2 || children.size() ==
3);
Review Comment:
Same as above — reverted to `2 || 3` since the FE no longer normalizes to 3
arguments.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/RegexpExtractAllArray.java:
##########
@@ -38,20 +39,32 @@
* Returns all matches of a regex pattern as an Array<String> instead of
a string-formatted array.
*/
public class RegexpExtractAllArray extends ScalarFunction
- implements BinaryExpression, ExplicitlyCastableSignature,
AlwaysNullable, PropagateNullLiteral {
+ implements ExplicitlyCastableSignature, AlwaysNullable,
PropagateNullLiteral {
public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
FunctionSignature.ret(ArrayType.of(VarcharType.SYSTEM_DEFAULT))
.args(VarcharType.SYSTEM_DEFAULT,
VarcharType.SYSTEM_DEFAULT),
FunctionSignature.ret(ArrayType.of(StringType.INSTANCE))
- .args(StringType.INSTANCE, StringType.INSTANCE)
+ .args(StringType.INSTANCE, StringType.INSTANCE),
+ FunctionSignature.ret(ArrayType.of(VarcharType.SYSTEM_DEFAULT))
Review Comment:
Good catch, thanks! Reworked in d387667: the FE no longer pads, and the BE
now registers both a 2-arg and a 3-arg variadic form (mirroring
regexp_replace's ThreeParamTypes/FourParamTypes), so an old FE keeps working
against a new BE during rolling upgrades.
##########
be/src/exprs/function/function_regexp.cpp:
##########
@@ -136,13 +136,17 @@ struct RegexpExtractEngine {
return false;
}
- // Match all occurrences and extract the first capturing group
- void match_all_and_extract(const char* data, size_t size,
+ // Match all occurrences and extract the capturing group with the given
index.
+ // index 0 means the whole match, 1 means the first capturing group (the
default), and so on.
+ void match_all_and_extract(const char* data, size_t size, int index,
std::vector<std::string>& results) const {
+ if (index < 0) {
+ return;
+ }
if (is_re2()) {
int max_matches = 1 + re2_regex->NumberOfCapturingGroups();
- if (max_matches < 2) {
- return; // No capturing groups
+ if (index >= max_matches) {
Review Comment:
Done — the index contract now matches Spark: an index outside [0,
number_of_capturing_groups] fails with InvalidArgument, with the valid range in
the message.
##########
be/src/exprs/function/function_regexp.cpp:
##########
@@ -159,9 +163,9 @@ 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() &&
!matches[index].empty()) {
Review Comment:
Done — non-participating groups now contribute empty strings instead of
being skipped (your `(a)|(b)` group-2 example is covered in the unit tests).
##########
be/src/exprs/function/function_regexp.cpp:
##########
@@ -174,8 +178,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() &&
matches[index].matched) {
Review Comment:
Done, both the re2 and the boost path keep empty groups now.
##########
regression-test/suites/query_p0/sql_functions/string_functions/test_string_function_regexp.groovy:
##########
@@ -255,4 +255,11 @@ 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_group_negative "select regexp_extract_all('abc',
'(b)', -1);"
+ 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_group_out_of_range "select
regexp_extract_all_array('hitdecisiondlist', '(i)(.*?)(e)', 3);"
Review Comment:
Expanded: column input (three group indexes over the test table), NULL
literals for string/pattern/index, illegal indexes via exception blocks, and
the non-participating-group case.
--
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]
