Repository navigation
GH-53159: [C++][Compute] Cache substring and regex matchers across invocations in scalar string kernels - #53160
ishaaaan17 wants to merge 2 commits into
Conversation
|
|
|
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! |
| return MatchSubstringImpl<Type, PlainSubstringMatcher>::Exec(ctx, batch, out, | ||
| matcher.get()); | ||
| if (!state->plain_substring_matcher) { | ||
| ARROW_ASSIGN_OR_RAISE(state->plain_substring_matcher, |
There was a problem hiding this 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?
|
Could we add a focused Google Benchmark case in The existing helper calls I'd suggest comparing:
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) |
|
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 i will work on these:
thanks again for the guidance! |
…oss invocations in scalar string kernels
…ng and regex matchers
03c487c to
e18a4e5
Compare
|
thanks for the detailed review and guidance! i have addressed all the points in two commits:
|
|
@ursabot please benchmark |
|
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. |
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_regexandmatch_like) rebuild their matcher on every call toExec. 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?
OptionsWrapper<MatchSubstringOptions>with aMatchSubstringState : public KernelStatethat caches the plain and regex matchers.MatchSubstringState::Get(ctx)so other kernels and tests that use the old accessor still work.PlainSubstringMatcher,PlainStartsWithMatcher,PlainEndsWithMatcherandRegexSubstringMatchernow reuse their compiled matchers across batches.MatchLikelooks at the SQL LIKE pattern and sets up the converter state once, instead of once per batch.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 throughCheckScalarUnary.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:
reviewed before submission by:
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.