[Feature](func) Support the third argument for regexp_extract_all and regexp_extract_all_array - #66050
[Feature](func) Support the third argument for regexp_extract_all and regexp_extract_all_array#66050ccl125 wants to merge 9 commits into
Conversation
… regexp_extract_all_array Add an optional group-index argument (Spark semantics, default 1) to regexp_extract_all and regexp_extract_all_array. Index 0 extracts the whole match, a positive index extracts the corresponding capturing group, an out-of-range index yields an empty result, and a negative index yields NULL. Two-argument calls are normalized in the FE by padding the default index, keeping existing behavior unchanged.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
| @Override | ||
| public RegexpExtractAll withChildren(List<Expression> children) { | ||
| Preconditions.checkArgument(children.size() == 2); | ||
| Preconditions.checkArgument(children.size() == 2 || children.size() == 3); |
There was a problem hiding this comment.
| Preconditions.checkArgument(children.size() == 2 || children.size() == 3); | |
| Preconditions.checkArgument(children.size() == 3); |
There was a problem hiding this 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.
| @Override | ||
| public RegexpExtractAllArray withChildren(List<Expression> children) { | ||
| Preconditions.checkArgument(children.size() == 2); | ||
| Preconditions.checkArgument(children.size() == 2 || children.size() == 3); |
There was a problem hiding this comment.
| Preconditions.checkArgument(children.size() == 2 || children.size() == 3); | |
| Preconditions.checkArgument(children.size() == 3); |
There was a problem hiding this comment.
Same as above — reverted to 2 || 3 since the FE no longer normalizes to 3 arguments.
The 2-arg constructor always pads the default group index, so every regexp_extract_all / regexp_extract_all_array node physically has 3 children. Tighten the withChildren assertion accordingly.
| FunctionSignature.ret(ArrayType.of(StringType.INSTANCE)) | ||
| .args(StringType.INSTANCE, StringType.INSTANCE) | ||
| .args(StringType.INSTANCE, StringType.INSTANCE), | ||
| FunctionSignature.ret(ArrayType.of(VarcharType.SYSTEM_DEFAULT)) |
There was a problem hiding this comment.
It's better not to change the parameter limit from two to a hard limit of three. I think support variable parameters is better.
Doris's version upgrades are generally rolling upgrades, first upgrading the backend (be) and then the frontend (fe).An old FE sends two arguments to a new BE, while an old FE pads two-argument SQL calls to three arguments and therefore cannot execute them on an new BE.
Could we keep the original two-argument expression shape and make the new BE accept both 2 and 3 arguments? When only two arguments are provided, the BE can simply use group index 1 as the default. This would preserve existing two-argument queries during a rolling upgrade
There was a problem hiding this 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.
| int max_matches = 1 + re2_regex->NumberOfCapturingGroups(); | ||
| if (max_matches < 2) { | ||
| return; // No capturing groups | ||
| if (index >= max_matches) { |
There was a problem hiding this comment.
The behavior for idx >= max_matches || index < 0 better refer to Spark throwing an error.
spark-sql (default)> SELECT regexp_extract_all('100-200, 300-400', '(\\d+)-(\\d+)', -1);
[INVALID_PARAMETER_VALUE.REGEX_GROUP_INDEX] The value of parameter(s) `idx` in `regexp_extract_all` is invalid: Expects group index between 0 and 2, but got -1. SQLSTATE: 22023There was a problem hiding this 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.
| 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()) { |
There was a problem hiding this comment.
should not judge matches[idx].empty() anymore:
spark-sql (default)> select regexp_extract_all('a b', '(a)|(b)', 2);
["","b"]There was a problem hiding this 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).
| 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) { |
There was a problem hiding this comment.
Done, both the re2 and the boost path keep empty groups now.
| 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);" |
There was a problem hiding this comment.
Please add more comprehensive tests, covering more cases, including but not limited to: including column input, including null, illegal index...
There was a problem hiding this comment.
btw, why this case named after _out_of_range?
I think (i)(.*?)(e) has three capturing groups, so index 3 is valid and should extract "e". The first out-of-range index is 4.
seems to be some issue in ur impl:
spark-sql (default)> select regexp_extract_all('hitdecisiondlist', '(i)(.*?)(e)', 3);
["e"]There was a problem hiding this 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.
There was a problem hiding this comment.
You were right, I miscounted — (i)(.*?)(e) has three capturing groups and index 3 extracts "e". Fixed the unit and regression cases; index 4 is now covered as the out-of-range error.
|
also remember to edit doc: https://github.com/apache/doris-website dev & 4.x versions of zh & en |
…dex semantics - Support both 2-arg and 3-arg forms at the BE (separate variadic registrations) instead of FE-side default padding, so old FE/new BE and new FE/old BE combinations keep working during rolling upgrades. - Align the group index contract with Spark: an index outside [0, number_of_capturing_groups] is an error instead of NULL/empty. - Keep empty strings for non-participating groups instead of skipping them, consistent with Spark. - Fix test group counting ((i)(.*?)(e) has three groups, index 3 is valid) and expand regression coverage: column input, NULL literals, illegal indexes, empty groups.
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review result: changes requested on d387667321b1ef9852fc3eefc3a89c04ba7c9778.
I found six blocking comment threads: the RE2 loop drops valid zero-length/terminal group-0 matches and can revisit a contextual match; the Boost loop can repeat zero-width matches and advance through UTF-8 bytes; the new bounds check regresses legacy two-argument/no-capture behavior and mixed nullable rows; the invalid-index BE unit fixture compiles a truncated regex; and the added column regressions query a table after it has been dropped.
Checkpoint conclusions:
- Goal/correctness: the optional-index goal is not yet met because the matcher and validation paths return incorrect results or errors for the cases above.
- Scope and interfaces: the implementation is focused. FE 2/3-argument construction, fixed signatures, child preservation, scalar translation, and the distinct BE
StringString/StringStringInt64registrations are structurally coherent. - Compatibility/upgrades: the BE-first rolling-upgrade wiring is structurally sound, but the implicit-index zero-capture regression still breaks existing two-argument behavior.
- Parallel and special paths: both string/array outputs, RE2/Boost engines, constness combinations, nullable wrapping, error cleanup, and test-table lifecycle were traced. The inline comments cover every substantiated issue found.
- Concurrency/lifecycle/configuration/persistence/data writes/observability: no applicable shared-state, lock, distributed-lifecycle, configuration, persistence, transaction, write-path, or observability issue was introduced.
- Performance: no distinct performance regression was substantiated beyond the incorrect traversal behavior already called out.
- Tests: static review only, as required by the review runner; no build or test was run. The added cases cover ordinary groups and literal NULL/error inputs, but do not cover zero-width/contextual traversal or mixed nullable rows, and the BE fixture plus table-lifetime bug prevent two intended test paths from executing.
- User focus:
review_focus.txtcontains no additional focus guidance; the full PR was reviewed. - Completion: three bounded rounds were completed. All final-round reviewers returned
NO_NEW_VALUABLE_FINDINGSagainst the exact six-comment payload, every candidate was accepted or dismissed with evidence, and the live PR head/base were reverified immediately before submission.
| 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()) { |
There was a problem hiding this 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.
| } | ||
| } | ||
| // Move position forward | ||
| auto offset = std::string(str_pos, str_size) |
There was a problem hiding this 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.
| 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()) { |
There was a problem hiding this 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.
| // 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) { |
There was a problem hiding this 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.
| auto col_str = ColumnString::create(); | ||
| col_str->insert_data("hitdecisiondlist", 16); | ||
| auto col_pattern = ColumnString::create(); | ||
| col_pattern->insert_data("(i)(.*?)(e)", 10); |
There was a problem hiding this 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.
| 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;" |
There was a problem hiding this 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.
…advancing - Only enforce the out-of-range group index error for the explicit three-argument form; two-argument calls on patterns without capturing groups keep returning an empty result. - Advance past re2 matches via the match pointer instead of a textual find, which could select an earlier identical substring. - Advance past boost zero-width matches from the match position, not from the old search origin. - Fix a truncated pattern literal in the invalid-index unit test and recreate the test table for the new column-input regression cases.
|
Thanks for the thorough scan! All five points are addressed in 1ca8dcf (pushed):
|
|
/review |
There was a problem hiding this comment.
Review result: changes requested on 1ca8dcfd949917d2f7da8c6004ec188f971b8abc.
I found one new blocking issue: after each non-empty match, both engines search the remaining suffix as a new regex subject, so anchors and left-context assertions change. The inline comment gives concrete RE2 and Boost failures.
Existing-thread status at this head:
- Fixed: RE2 textual-occurrence advancement, Boost delayed-zero-width origin advancement, legacy two-argument/no-capture behavior, the malformed invalid-index unit fixture, and the dropped-table regression setup.
- Still blocking and already reported (not duplicated here): RE2 empty/terminal group-0 matching, encoding-safe Boost advancement after empty matches, and mixed nullable-row index validation.
Checkpoint conclusions:
- Goal/correctness: not met yet; the new group-index path still returns incorrect match sets for contextual patterns, in addition to the unresolved existing-thread cases.
- Scope/interfaces: the six-file change is focused. FE two-/three-argument signatures, child preservation, nullable return shapes, and the BE's exact typed registrations are structurally coherent.
- Compatibility/upgrades: two-argument old-FE/new-BE calls and new-FE/fully-upgraded-BE calls resolve correctly under the BE-first rolling order; no separate serialization-compatibility issue was found.
- Parallel/special paths: both string/array handlers, RE2/Boost engines, constant/nonconstant pattern/index shapes, nullable wrapping, invalid-index status flow, and test-table lifecycle were traced. The new inline plus existing threads cover every substantiated correctness issue.
- Concurrency/lifecycle/configuration/persistence/data writes/observability: no new shared-state, lock, lifecycle, configuration, persistence, transaction, write-path, or observability issue applies.
- Performance: no distinct performance regression was substantiated.
- Tests: static review only, as required by the review runner; no build or test was run. Current-head fixes make the intended unit/regression paths structurally reachable and deterministic, but coverage is still missing for suffix-context matching and the already-reported empty/multibyte/mixed-null paths.
- Documentation: the user-visible feature has a linked website PR in the PR description.
- User focus:
review_focus.txtcontains no additional guidance; the full PR was reviewed. - Completion: three bounded rounds were completed. All final-round reviewers returned
NO_NEW_VALUABLE_FINDINGSagainst this exact one-comment payload and body, every candidate was accepted/dismissed/deduplicated with evidence, and the live PR head/base were reverified immediately before submission.
| pos += offset + matches[0].size(); | ||
| // Move position forward. matches[0] points into the searched string, | ||
| // so its address gives the exact match offset — no textual find needed. | ||
| pos += (matches[0].data() - str_pos) + matches[0].size(); |
There was a problem hiding this comment.
[P1] Preserve the original regex subject between matches
This exact-pointer advance fixes the earlier textual-find bug, but the next iteration still passes StringPiece(str_pos, str_size) (and Boost [search_start, search_end)) as a brand-new subject. That changes left context after any non-empty match: regexp_extract_all_array('aa', '^a', 0) treats the second byte as another beginning and returns ["a","a"] instead of Spark's single anchored match; with extended regex, regexp_extract_all_array('ab', 'a|(?<=a)b', 0) loses the a before search_start and misses the valid b. This is distinct from the existing zero-width/progress threads. Please keep the original subject/range and advance a search offset/flags that preserve beginning and previous-character context.
There was a problem hiding this comment.
Fixed in cf47ba1 — the re2 loop now calls Match on the original subject with an advancing start position (so ^ and left context refer to the original string), and the boost loop keeps the original begin reachable with match_prev_avail and suppresses bol at the advanced origin with match_not_bol. Added ('aa', '^a', 0) -> ['a'] as a regression case.
There was a problem hiding this comment.
The RE2 side and the simple ^ case are fixed, but the Boost loop still restarts buffer-anchor semantics after the first match. The advanced call uses match_prev_avail | match_not_bol; Boost documents that match_prev_avail makes match_not_bol ignored, defines match_not_bob for \A, and uses match_prev_avail | match_not_bob in its repeated-regex_search example.
With enable_extended_regex=true, regexp_extract_all_array('ab', 'a|(?<=a)\\Ab', 0) can therefore treat the suffix at b as a new buffer and return ["a","b"] instead of ["a"]. Please preserve beginning-of-buffer semantics for the advanced search and add a Boost-path regression. This is a follow-up to the same original-subject issue rather than a new thread.
There was a problem hiding this comment.
Fixed in 4737a9d — switched the boost flags to the documented repeated-search idiom match_prev_avail | match_not_bob (with match_prev_avail set, match_not_bol is ignored; match_not_bob keeps \A anchored to the start of the original buffer).
re2 now searches the original subject with an advancing start position instead of a fresh substring, so `^` stays anchored to the beginning of the original string; the boost path keeps the original begin reachable via match_prev_avail and suppresses bol at the advanced origin via match_not_bol.
|
/review |
There was a problem hiding this comment.
Review result: changes requested on cf47ba19e4de62b5f1f78008ad318cf9075c34ed.
No new inline threads were opened because every current-head correctness issue deduplicates to an existing thread. I added one follow-up reply to the attempted Boost subject-preservation fix.
Current-head status:
- Fixed: exact RE2 match-offset advancement, legacy two-argument/no-capture behavior, the malformed invalid-index unit fixture, the dropped-table regression setup, and RE2's original-subject search.
- Still blocking: RE2 empty/terminal group-0 matches, encoding-safe Boost zero-width progress, and mixed nullable-row index validation.
- Partially fixed: original-subject preservation. The RE2 path and simple
^case are corrected, but the Boost suffix search still lacks beginning-of-buffer preservation for\A; the thread reply gives a concrete forced-Boost case and the required flag direction.
Checkpoint conclusions:
- Goal/correctness: not met yet. The optional group-index path still loses valid empty/terminal matches, can enter UTF-8 continuation bytes after a Boost zero-width match, can fail NULL propagation before explicit-index validation, and can change Boost buffer-anchor semantics after a prior match.
- Scope and interfaces: the six-file change is focused. FE two-/three-argument constructor discovery, signature selection, child preservation, and the distinct BE
StringString/StringStringInt64factory registrations are coherent. - Compatibility/upgrades: the BE-first rolling-upgrade wiring is sound for old-FE/new-BE and legacy two-argument new-FE/old-BE calls; explicit three-argument syntax appropriately requires upgraded BEs.
- Parallel and special paths: both string/array handlers, RE2/Boost engines, constant/nonconstant pattern/index shapes, nullable wrapping, invalid-index status flow, and test-table lifecycle were traced. The existing threads plus the follow-up reply cover every substantiated issue.
- Concurrency/lifecycle/configuration/persistence/data writes/observability: no new shared-state, lock, lifecycle, configuration, persistence, transaction, write-path, or observability issue applies.
- Performance: no distinct performance regression was substantiated beyond the incorrect repeated-search behavior already covered by the correctness threads.
- Tests: static review only, as required by the review runner; no build or test was run. The repaired unit fixture and recreated regression table make their intended cases structurally reachable and deterministic, and the new
.outrows match the inserted data. Coverage is still missing for the live empty/terminal, multibyte Boost, mixed-nullable, and Boost buffer-anchor failures. - Documentation: the user-visible feature has a linked website PR in the PR description.
- User focus:
review_focus.txtcontains no additional guidance; the full PR was reviewed. - Completion: three bounded rounds were completed. All final-round reviewers returned
NO_NEW_VALUABLE_FINDINGSagainst this exact reply and review payload, every candidate was accepted, dismissed, or deduplicated with evidence, and the live PR head/base were reverified immediately before submission.
Boost ignores match_not_bol once match_prev_avail is set; the documented repeated-regex_search idiom is match_prev_avail | match_not_bob, which keeps `\A` anchored to the start of the original buffer.
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
|
Hi @linrrzqqq @morrySnow, all review feedback has been addressed (latest: the boost repeated-search flags in 4737a9d). Could someone help trigger |
|
/review |
There was a problem hiding this comment.
Review result: changes requested on 4737a9dc3e37552c486c92980202530c6fdd2606.
No new inline threads were opened because every substantiated current-head issue deduplicates to an existing review thread.
Current blockers:
- RE2 still drops successful empty and terminal group-0 matches.
- Boost zero-width progress is still byte-wise and unsafe for multibyte subjects.
- Mixed nullable rows can still validate an invalid payload index before NULL propagation.
Resolved since the prior review: exact RE2 match-offset advancement, legacy two-argument/no-capture handling, the invalid-index unit-test literal, regression-table recreation, original-subject searching, and Boost beginning-of-buffer preservation with match_not_bob.
Checkpoint conclusions:
- Goal and correctness: not yet met because the three current-head defects above remain blocking.
- Scope and interfaces: the six-file change is focused; FE constructor discovery, coercion, rebuild, translation, and BE two-/three-argument registrations are coherent.
- Compatibility: the physical two-argument form remains available for old-FE/new-BE rollout, and the new three-argument form maps to a distinct BE signature. The disputed implicit-group behavior is a same-matcher-line duplicate of an existing thread, not a separate reportable instance.
- Parallel and special paths: string/array outputs, RE2/Boost engines, constant/nonconstant arguments, nullable execution, and error paths were traced. The surviving gaps are already represented by the linked threads.
- Concurrency, lifecycle, configuration, persistence, transactions, data writes, observability, and FE-BE session variables: no applicable new mechanism or shared state is introduced; regex state remains thread-local.
- Performance: no distinct material regression was found beyond the existing correctness blockers.
- Tests and results: static review only; this runner forbids builds and tests. The changed expected output is deterministic and the table setup is valid. Missing mixed-nullable, terminal/zero-width, and multibyte coverage corresponds to the existing blockers.
- Documentation and focus: the PR links a website documentation change; no extra review focus was supplied, so the full PR was reviewed.
- Completion: two independent review rounds were completed. In Round 2, the full BE, full FE/tests, and risk-focused reviewers all returned
NO_NEW_VALUABLE_FINDINGS; every candidate was independently verified and adjudicated.
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 17000 ms |
TPC-DS: Total hot run time: 80644 ms |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Cover the two- and three-argument forms, signature list, and withChildren arity handling of RegexpExtractAll and RegexpExtractAllArray to feed the FE coverage gate.
|
Thanks for triggering buildall! Everything passed except the FE coverage gate (our FE change is only declarative signatures, so the increment had no UT coverage). Added a dedicated FE unit test for both function classes in 2817282 covering constructors/signatures/withChildren for the 2-arg and 3-arg forms. Could you help trigger |
| @@ -159,29 +165,42 @@ struct RegexpExtractEngine { | |||
There was a problem hiding this comment.
For the newly supported
idx = 0, this drops valid zero-length matches before the selected group is appended, andwhile (pos < size)also skips a terminal match. For example,regexp_extract_all('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.
fix this
There was a problem hiding this comment.
also fix : #66050 (comment) and #66050 (comment)
There was a problem hiding this comment.
Fixed in 1bcc272 — explicit group-index calls now follow Spark exactly: zero-width matches are emitted ('b','a*',0 -> ['',''] and '' -> ['']), one terminal-position search is allowed, and advancement is UTF-8-safe (whole characters, no continuation-byte splits). The legacy two-argument form keeps the old skip behavior, covered by a dedicated case. New unit cases added for all three examples.
There was a problem hiding this comment.
Also fixed in 1bcc272 — input null maps are now merged before validation, so (NULL,'(a)',99), ('hitdecisiondlist','(i)(.*?)(e)',3) yields NULL for the first row and ['e'] for the second instead of aborting on the garbage index. A dedicated two-row nullable-block test covers it. The UTF-8-unsafe advance in the boost path is fixed the same way (whole-character steps via get_utf8_byte_length).
- Explicit group-index calls now emit successful zero-width matches
(including one terminal-position match) and advance one full UTF-8
character, matching Spark ('b','a*',0 -> ["",""]). The legacy
two-argument form keeps skipping zero-width matches.
- Merge input null maps before validation so a row with a null
string/pattern/index yields NULL for that row instead of aborting
the query on an out-of-range garbage index.
- The boost zero-width advance now moves by whole UTF-8 characters
instead of a raw byte, and both engines allow a terminal search.
|
/review |
There was a problem hiding this comment.
Review result: changes requested on 1bcc27269761970bc7213cfebe14e13194b18811.
I found four new issues: two legacy two-argument behavior regressions, an RE2 later-origin zero-width progress bug, and a mandatory clang-format v16 failure. The mixed-null validation failure remains present on this head, but it is already fully covered by the existing thread, so I did not duplicate it.
Checkpoint conclusions:
- Goal and proof: the optional index is wired through both FE expressions, both BE result forms, and distinct two-/three-argument factory keys, but the runtime goal is not met. Existing two-argument results change for nonparticipating/empty captures and subject-sensitive repeated matching; explicit-index RE2 searches can duplicate a zero-width match found after the current origin; and the existing mixed-null path still validates a garbage nested index before NULL propagation.
- Scope and focus: the seven-file change is focused on the shared regexp engine, its two output handlers/registrations, FE binding, and tests.
review_focus.txtcontains no additional guidance; the full PR was reviewed. - Concurrency: no new thread, lock, atomic, or cross-thread mutable state is introduced.
- Lifecycle: constant patterns retain the established thread-local compiled engine, dynamic patterns use per-row scoped ownership, and no cross-TU static initialization dependency or abnormal release path was added.
- Configuration: no configuration item is added; the existing
enable_extended_regexswitch is reused. - Compatibility: the separate
StringStringandStringStringInt64registrations preserve old-FE/new-BE two-argument routing and new-FE/new-BE three-argument routing, and nullable type-family lookup is coherent. Result compatibility is nevertheless broken by the two legacy-arities issues called out inline. No storage format, serialized symbol, or protocol field changes are introduced. - Parallel and special paths: both string/array outputs, RE2/Boost engines, legacy/explicit arities, constant/vector pattern and index shapes, zero-width/terminal matches, capture participation, nullable inputs, error propagation, and output-offset finalization were traced. Boost's current returned-match-relative progress and original-context flags add no separate issue; the remaining mixed-null failure is the existing-thread duplicate noted above.
- Tests and expected results: the FE tests, BE unit cases, regression labels, expected rows, ordering, and table lifecycle were reviewed. Concrete missing cases correspond to the inline legacy nonparticipating/anchor regressions and later-origin RE2 boundary case. The new mixed-null unit test cannot pass through the default nullable-unwrapping path already described in the existing thread. No build or test suite was run because this review runner forbids builds; the read-only clang-format v16 check was run and fails on both changed C++ files, while their base revisions pass.
- Observability: no new distributed or operational path needs logs or metrics; existing status/warning propagation is otherwise sufficient.
- Persistence, transactions, and writes: no EditLog, failover, transaction, storage-visibility, delete-bitmap, or production data-write path is involved.
- FE-BE variables: the index is an ordinary scalar-function child carried by existing translation; no new session or thrift variable requires propagation.
- Performance and memory: no distinct hot-path, allocation-accounting, ownership, or memory-safety issue was substantiated beyond the incorrect repeated-search behavior already reported.
- Completion: three bounded review rounds were completed. All three final-round reviewers returned
NO_NEW_VALUABLE_FINDINGS; every candidate was accepted, dismissed with evidence, or deduplicated, so this review is converged rather than capped/incomplete. The live head/base and current review threads were reverified immediately before submission.
| bool success = re2_regex->Match(re2::StringPiece(str_pos, str_size), 0, str_size, | ||
| // Search within the original subject starting from pos, so `^` stays | ||
| // anchored to the beginning of the original string. | ||
| bool success = re2_regex->Match(re2::StringPiece(data, size), pos, size, |
There was a problem hiding this comment.
[P1] Preserve suffix anchoring for the legacy arity
Searching the original subject is needed for the new explicit-index semantics, but this shared call also changes existing two-argument results. Before this PR, regexp_extract_all('aa', '^(a)') searched each remaining suffix as a new subject and returned ['a','a']; this now keeps ^ tied to offset 0 and returns only ['a'] (the array form changes too). The existing original-subject thread covered an explicitly supplied index, while the PR promises the no-index form remains unchanged. Please split the cursor semantics by arity, or explicitly declare and test the legacy behavior change.
There was a problem hiding this comment.
Fixed in ec1c238 — the legacy two-argument form now runs the pre-change substring loop verbatim, so ^ anchoring and suffix behavior are byte-identical to before this PR; only the explicit group-index form searches the original subject.
| 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()) { |
There was a problem hiding this comment.
[P1] Preserve nonparticipating groups for the legacy arity
This unconditional append also runs when emit_empty_matches is false, so it changes existing two-argument queries. For example, regexp_extract_all('a b', '(a)|(b)') previously skipped the unmatched default group 1 on the b match and returned ['a']; it now appends an empty element and returns ['a',''] (the array variant changes likewise). The existing thread about preserving empty groups covered an explicitly supplied index, whereas the PR promises the no-index form remains unchanged. Please retain the former unmatched/empty-group filtering for the two-argument path and add a legacy-arity case for both outputs.
There was a problem hiding this comment.
Fixed in ec1c238 — the legacy loop again appends only the first group when non-empty (the unconditional append now lives only in the Spark loop used by explicit group-index calls).
| break; | ||
| } | ||
| // Advance one full UTF-8 character, never into a continuation byte. | ||
| pos += get_utf8_byte_length(static_cast<uint8_t>(data[pos])); |
There was a problem hiding this comment.
[P1] Advance from the returned RE2 empty-match offset
An unanchored RE2 search can return a zero-width match strictly after pos, but this advances from data[pos] instead of from whole.data(). For example, regexp_extract_all_array('ab', '\\b', 0) should emit the two boundaries at offsets 0 and 2. After the first match sets pos to 1, the next search finds offset 2; this line advances only to 2, so the following iteration emits that same boundary again. The earlier progress thread covered the Boost branch, which now correctly uses matches[0].first; please compute the RE2 match offset from whole.data() - data before the UTF-8 step and add this later-than-origin case for both outputs.
There was a problem hiding this comment.
Fixed in ec1c238 — zero-width advancement now starts from the returned match location (whole.data() + one UTF-8 character), not from data[pos].
| break; | ||
| } | ||
| search_start += 1; | ||
| search_start = |
There was a problem hiding this comment.
[P2] Apply the mandatory clang-format v16 pass
The required formatter rejects both changed BE files: clang-format-16 --dry-run --Werror be/src/exprs/function/function_regexp.cpp be/test/exprs/function/function_like_test.cpp reports this cursor wrapping plus the new include/null-map code and several changed unit-test lines. The base revisions of both files pass the same check, so these violations are introduced here. Please run build-support/clang-format.sh on the changed C++ files so the repository's formatting gate can pass.
There was a problem hiding this comment.
Done in ec1c238 — both BE files now pass clang-format-16 --dry-run --Werror locally.
The shared loop leaked Spark semantics into the legacy two-argument form: empty/non-participating groups were appended where the old code skipped them, and the original-subject search changed `^` anchoring. Restore the pre-change loop verbatim for the legacy form (substring searches, zero-width skipped, first-group-only non-empty appends) and keep Spark semantics in a dedicated loop for the explicit group-index form. Empty-match advancement now starts from the returned match location. Files formatted with clang-format 16.
|
/review |
There was a problem hiding this comment.
Automated review result: request changes. The review converged after the third full round with two new inline correctness findings. A third confirmed failure—the mixed-null row whose nested invalid index is validated after nullable wrappers are stripped—remains covered by existing discussion r3681145562, so I did not duplicate it inline.
Critical-checkpoint conclusions:
- Goal and scope: The patch is focused on adding the optional Spark-style group index to both result representations while retaining the two-argument form. No additional user focus was supplied.
- Correctness and special conditions: Ordinary group selection, nonparticipating groups, ASCII zero-width/terminal matches, and checked index errors are consistent. The two new inline findings cover lost Boost previous-match state and supplementary-character zero-width iteration; the existing nullable-row failure is still unresolved.
- Compatibility and parallel paths: Exact FE constructors/signatures and BE family keys resolve both arities without collision. The supported old-FE/new-BE rolling-upgrade direction is preserved, and the string/array outputs share the same implementation.
- Concurrency and lifecycle: Compiled regex state is function-context thread-local. No new locks, cross-thread state, static-initialization dependency, startup/shutdown lifecycle, transaction/persistence, or data-write path is involved.
- Configuration and FE/BE propagation: No new variable is introduced. The existing
enable_extended_regexpath selects Boost and exposes the\Gfinding; no separate propagation defect was found. - Tests and result provenance: The added FE, BE, and regression cases are reachable and their inspected ordinary outputs are deterministic. However, the mixed-null BE test should fail on the normal wrapper path, and there is no explicit-index Boost
\Gor supplementary-plane zero-width case. Per the review-runner instructions, repository builds/tests were not run. - Performance, memory, and observability: No distinct material regression survived review; work and result allocation remain proportional to emitted matches. Existing checked statuses/warnings are sufficient, with no new observability issue.
Please resolve both inline findings and the existing nullable-row thread before approval.
| } | ||
| // Advance one full UTF-8 character FROM THE MATCH LOCATION, never | ||
| // into a continuation byte and never rescanning the same match. | ||
| pos = (whole.data() - data) + |
There was a problem hiding this comment.
[P1] Preserve Spark's supplementary-character empty matches
Spark delegates this loop to Java Matcher.find(), whose progress after an empty match is one UTF-16 code unit. For regexp_extract_all_array('😀a', 'a*', 0), Java emits four entries: empty matches at UTF-16 offsets 0 and 1, a at 2:3, and the terminal empty match. This branch instead advances from byte 0 over the whole four-byte UTF-8 character, so it returns only ["", "a", ""]; the Boost branch makes the same jump. Since the explicit-index path is documented as Spark semantics, please either emulate the UTF-16 iteration positions without passing invalid byte offsets to the engines or narrow that contract, and add supplementary-character cases for both outputs.
| // match_not_bob keeps `\A` anchored to the start of the original | ||
| // buffer (Boost's documented repeated-regex_search idiom; with | ||
| // match_prev_avail set, `^` is decided by the preceding character). | ||
| while (search_start <= search_end && |
There was a problem hiding this comment.
[P1] Preserve previous-match state for \G
\G is stateful: Spark reuses one Java Matcher, so on ab the pattern \G matches only at offset 0; after that zero-width match, find() advances while the previous match still ended at 0. This loop instead starts a fresh Boost search at each new search_start, so each suffix origin is treated as a new continuation point and the array form emits empty matches at offsets 0, 1, and 2 (the string form shares the same engine). Please preserve continuation state across searches or reject \G, and add a Boost-path regression for this case.
4 participants
What problem does this PR solve?
Issue Number: related #48203 (Spark:
regexp_extract_allfor the third argument)Problem Summary:
regexp_extract_all/regexp_extract_all_arrayonly accepted 2 arguments and always extracted the first capturing group. This PR adds the optional thirdgroupargument, following Spark semantics:regexp_extract_all(str, pattern)— unchanged, extracts group 1regexp_extract_all(str, pattern, 0)— extracts the whole match (also works for patterns without capturing groups)regexp_extract_all(str, pattern, N)— extracts capturing group N[0, number_of_capturing_groups]→ error (same as Spark); non-participating groups yield empty stringsImplementation:
RegexpExtractAll/RegexpExtractAllArraygain 3-arg signatures; the 2-arg form stays a 2-child node (no default padding) so old-FE/new-BE combinations keep working during rolling upgradesregexp_replace's ThreeParamTypes/FourParamTypes);RegexpExtractEngine::match_all_and_extracttakes a group index; illegal indexes (outside[0, number_of_capturing_groups]) raiseINVALID_ARGUMENT, and non-participating groups contribute empty strings — both consistent with SparkRelease note
Support the third argument (group index) for
regexp_extract_allandregexp_extract_all_array.Check List (For Author)
Test
New BE unit cases in
function_like_test.cppcover group 0 / other groups / out-of-range / negative index for both functions; existing 2-arg cases were ported to the normalized 3-arg form with unchanged expectations. New regression cases added totest_string_function_regexp.Behavior changed:
2-argument calls keep the exact previous behavior (group 1 extraction, empty result when the pattern has no capturing group).
Does this need documentation?
Check List (For Reviewer who merge this PR)