[ishaaaan17]

rationale for this change

in cpp/src/arrow/compute/kernels/scalar_string_ascii.cc, the string matching kernels (match_substring, starts_with, ends_with, match_substring_regex and match_like) rebuild their matcher on every call to Exec. for regex matchers that means the RE2 DFA is recompiled for every batch in a stream or chunked array. plain matchers reallocate and rebuild the Knuth-Morris-Pratt prefix table each time.

this also closes out the existing todo in MatchSubstring:
// TODO Cache matcher across invocations (for regex compilation).

what changes are included in this PR?

• replaced OptionsWrapper<MatchSubstringOptions> with a MatchSubstringState : public KernelState that caches the plain and regex matchers.
• kept MatchSubstringState::Get(ctx) so other kernels and tests that use the old accessor still work.
• PlainSubstringMatcher, PlainStartsWithMatcher, PlainEndsWithMatcher and RegexSubstringMatcher now reuse their compiled matchers across batches.
• MatchLike looks at the SQL LIKE pattern and sets up the converter state once, instead of once per batch.
• ran clang-format.

are these changes tested?

yes, by the existing kernel tests in cpp/src/arrow/compute/kernels/scalar_string_test.cc. they run the matching kernels over chunks, slices and scalars through CheckScalarUnary.

are there any user-facing changes?

no public api changes.

was ai used for this PR?

in line with the ai generation guidelines, here is how ai was used.

pr code and description written by:

• human
• ai

reviewed before submission by:

• human
• ai
• not reviewed

i used an llm assistant to look at different ways of caching the matchers and to draft the first version of the changes. i then went through and reworked the design myself, checked that the old state accessors still work, made sure the options are stored safely, and formatted the code.

• GitHub Issue: [C++][Compute] Cache substring and regex matchers across invocations in scalar string kernels #53159

[github-actions]

⚠️ GitHub issue #53159 has been automatically assigned in GitHub to PR creator.

[ishaaaan17]

hi team,

a gentle reminder on this PR, whenever someone gets time, kindly approve the CI workflows.

i am happy to take any feedback on the design, change the caching approach, or share some more small benchmarks for reusing the string matcher across record batches, if you want. thanks!

[dmitry-chirkov-dremio]

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?

[dmitry-chirkov-dremio]

Could we add a focused Google Benchmark case in cpp/src/arrow/compute/kernels/scalar_string_benchmark.cc and share before/after results?

The existing helper calls CallFunction afresh each iteration, which recreates kernel state. To measure this optimization, please initialize one FunctionExecutor or bind one expression, then reuse it across multiple batches.

I'd suggest comparing:

• One batch versus many batches, keeping the total input size fixed.
• A simple and a more expensive regex, plain substring matching, and LIKE's fast path versus regex fallback.
• Total execution including initialization, plus steady-state execution after the first batch.

A separate benchmark commit would make this easy to reproduce against both revisions. I'd prefer retaining a small representative benchmark for future regression tracking but I'm not sure about general policies of "bloating" the benchmarks.

p.s. please benchmark the version after fixing concurrent initialization (see my other comment)

[ishaaaan17]

hi @dmitry-chirkov-dremio,

thank you very much for the detailed review, and for catching the race condition in Acero!

you are right. when the worker threads evaluate the same bound expression on different batches at the same time, they share kernel_state, and if the matcher is initialized on the first batch without any synchronization, it is a data race.

i will work on these:

1. add synchronization for the matcher initialization. i can use a std::mutex with double-checked locking, or follow any specific pattern @lriggs suggests.
2. add a unit test where multiple threads use a fresh kernel state for the first time at the same time.
3. add the benchmark you asked for in scalar_string_benchmark.cc, comparing a single batch against reusing across multiple batches using FunctionExecutor, and share the before/after numbers.

thanks again for the guidance!

[ishaaaan17]

hi @dmitry-chirkov-dremio,

thanks for the detailed review and guidance! i have addressed all the points in two commits:

1. thread-safe lazy initialization

   • fixed the race in MatchSubstringState using double-checked locking in GetOrInitMatcher, with std::atomic acquire/release and a std::mutex:
     • fast path (normal case across batches): only one atomic acquire load, no mutex contention.
     • slow path (first batch race): takes the mutex and does a relaxed double check before building and storing the matcher.
   • changed MatchLike to call ExecWithState(ctx, batch, out, like_state.get()) directly, so there is no ctx->SetState() change at runtime anymore.
   • fixed the formatting in enum class LikeKind so the clang-format CI check passes.

2. concurrent first-use tests

   • added MatchSubstringConcurrentFirstUse and MatchLikeConcurrentFirstUse to TestBaseBinaryKernels in scalar_string_test.cc.
   • the test binds an expression that is not initialized yet, starts 16 threads that wait on an atomic barrier, and then all of them run ExecuteScalarExpression(bound_expr, batch) on the first batch together, to check there is no data race or use-after-free during initialization.

3. multi-batch google benchmark

   • added MatcherReusedBenchmark in scalar_string_benchmark.cc, in a separate commit on top as you asked.
   • it uses GetFunctionExecutor with 1, 8 and 64 batches, keeping the total input fixed at 1M elements. it covers plain substring (match_substring), simple regex, expensive regex (match_substring_regex), the LIKE fast path, and the LIKE regex fallback (match_like).

[ishaaaan17]

@ursabot please benchmark

[rok]

Benchmark runs are scheduled for commit e18a4e5. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete.

[dmitry-chirkov-dremio]

Benchmark runs are scheduled for commit e18a4e5. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete.

@ishaaaan17 could you review the benchmark results, please?

[ishaaaan17]

hi @dmitry-chirkov-dremio,

i checked the previous benchmark run on Buildkite (build #1297).

it failed during the C++ compilation, because scalar_string_benchmark.cc was missing #include "arrow/compute/function.h". because of that, FunctionExecutor was an incomplete type when calling Execute({batch}).

i have added the missing include in the benchmark commit and pushed the new revision (4c041ed).

@ursabot please benchmark

[lriggs]

I think there might be an issue here with the create/run initialization and ordering.

This uses memory_order_relaxed and the ready.store(true) fires immediately after thread construction.

Thread creation and OS scheduling are not instantaneous. Some threads may not have yet entered (or even started) the spin-wait when ready becomes true. In that case they run sequentially after the first-use is already complete, and the test never actually exercises a concurrent first-init race.

You could fix this by using a std::atomic countdown latch: have each thread decrement a counter when it's ready, spin until the counter reaches zero, then proceed. This guarantees all threads are spinning before any executes. (C++20 std::latch or std::barrier work too)

std::atomic waiting{0};
std::atomic go{false};
for (int i = 0; i < kNumThreads; ++i) {
threads.emplace_back(&, i {
waiting.fetch_add(1, std::memory_order_relaxed);
while (!go.load(std::memory_order_acquire)) {}
results[i] = ExecuteScalarExpression(bound_expr, batch);
});
}
// Wait for all threads to reach the barrier
while (waiting.load(std::memory_order_relaxed) < kNumThreads) {}
go.store(true, std::memory_order_release);

[ishaaaan17]

thanks for pointing this out @lriggs! you are right about the thread scheduling delay, and about making sure all the threads really race on the first initialization together.

i have changed the test synchronization to use an atomic barrier: a waiting counter and a go flag with acquire/release semantics, and std::this_thread::yield() inside the spin-waits so no core is starved. i made the same change in MatchLikeConcurrentFirstUse.

pushed the updated revision.

[lriggs]

thanks

[lriggs]

Can you add a similar test for RE2 using call("match_substring_regex". for example?

[ishaaaan17]

done! i added MatchSubstringRegexConcurrentFirstUse, which uses call("match_substring_regex", ...) with the same atomic countdown barrier, under #ifdef ARROW_WITH_RE2.

[lriggs]

thanks

[lriggs]

Since this is repeated across the three tests it would be nice to generalize to a helper function if its not too much trouble. But Im ok with it was it is also.