dmitry-chirkov-dremio commented on code in PR #53160:
URL: https://github.com/apache/arrow/pull/53160#discussion_r4298102457
##########
cpp/src/arrow/compute/kernels/scalar_string_ascii.cc:
##########
@@ -1371,80 +1408,98 @@ struct MatchSubstring {
template <typename Type>
struct MatchSubstring<Type, RegexSubstringMatcher> {
static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
- // TODO Cache matcher across invocations (for regex compilation)
- const bool is_utf8 = is_string_or_string_view(batch[0].type()->id());
- ARROW_ASSIGN_OR_RAISE(auto matcher, RegexSubstringMatcher::Make(
- MatchSubstringState::Get(ctx),
is_utf8));
- return MatchSubstringImpl<Type, RegexSubstringMatcher>::Exec(ctx, batch,
out,
-
matcher.get());
+ auto* state = MatchSubstringState::GetState(ctx);
+ if (!state->regex_substring_matcher) {
+ const bool is_utf8 = is_string_or_string_view(batch[0].type()->id());
+ ARROW_ASSIGN_OR_RAISE(state->regex_substring_matcher,
+ RegexSubstringMatcher::Make(state->options,
is_utf8));
+ }
+ return MatchSubstringImpl<Type, RegexSubstringMatcher>::Exec(
+ ctx, batch, out, state->regex_substring_matcher.get());
}
};
#endif
template <typename Type>
struct MatchSubstring<Type, PlainSubstringMatcher> {
static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
- auto options = MatchSubstringState::Get(ctx);
- if (options.ignore_case) {
+ auto* state = MatchSubstringState::GetState(ctx);
+ if (state->options.ignore_case) {
#ifdef ARROW_WITH_RE2
- const bool is_utf8 = is_string_or_string_view(batch[0].type()->id());
- ARROW_ASSIGN_OR_RAISE(auto matcher, RegexSubstringMatcher::Make(options,
is_utf8,
-
/*literal=*/true));
- return MatchSubstringImpl<Type, RegexSubstringMatcher>::Exec(ctx, batch,
out,
-
matcher.get());
+ if (!state->regex_substring_matcher) {
+ const bool is_utf8 = is_string_or_string_view(batch[0].type()->id());
+ ARROW_ASSIGN_OR_RAISE(
+ state->regex_substring_matcher,
+ RegexSubstringMatcher::Make(state->options, is_utf8,
/*literal=*/true));
+ }
+ return MatchSubstringImpl<Type, RegexSubstringMatcher>::Exec(
+ ctx, batch, out, state->regex_substring_matcher.get());
#else
return Status::NotImplemented("ignore_case requires RE2");
#endif
}
- ARROW_ASSIGN_OR_RAISE(auto matcher, PlainSubstringMatcher::Make(options));
- return MatchSubstringImpl<Type, PlainSubstringMatcher>::Exec(ctx, batch,
out,
-
matcher.get());
+ if (!state->plain_substring_matcher) {
+ ARROW_ASSIGN_OR_RAISE(state->plain_substring_matcher,
Review Comment:
This lazy initialization needs synchronization: Acero can evaluate the same
bound expression concurrently across batches, sharing its `kernel_state`. Two
threads can both observe an empty cache and assign different matchers,
destroying a matcher while another thread is using it.
Please add a concurrent first-use regression test; sequential chunked-array
tests would not catch this.
@lriggs do we already have a pattern to solve this?
--
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]
