GH-50615: [C++] Reduce code generation for string kernels#50616
Conversation
Some string kernels are generated twice for the corresponding binary and string type (for example binary/utf8, large_binary, large_utf8). Most of the time we can cut down on the code duplication to reduce compile times and binary size slightly.
3e700f2 to
0c56c1f
Compare
|
@fenfeng9 Would you like to review this? |
zanmato1984
left a comment
There was a problem hiding this comment.
Overall this looks good to me. I found one small non-blocking test coverage gap that may be worth closing before merge.
This PR now relies on the runtime input type to choose UTF-8 vs Latin-1 semantics after string/binary kernels share the same physical binary implementation. The new tests cover this well for the regex-named kernels, but I don’t think they directly cover two related paths:
match_like, especially the newMatchLikeConstants::Instance(is_utf8)two-slot cache.- Literal ignore-case kernels such as
match_substring,starts_with,ends_with,find_substring, andcount_substring.
A small binary-vs-utf8 non-ASCII case would help prove that the runtime is_utf8 selection cannot cross wires. For example, pattern = "ÀB" or "%ÀB%" with ignore_case = true should not match lowercase àb on binary input, but should match it on utf8 input.
| ARROW_ASSIGN_OR_RAISE( | ||
| const auto like_constants, | ||
| MatchLikeConstants::Instance( | ||
| /*is_utf8=*/is_string_or_string_view(batch[0].type()->id()))); |
There was a problem hiding this comment.
Could we add a targeted binary-vs-utf8 test for this runtime selection? Since string and binary now share the same physical MatchLike implementation, this cache is the one place where accidentally selecting the wrong UTF-8 / Latin-1 slot would be hard to catch with ASCII-only inputs.
For example, pattern = "%ÀB%" with ignore_case = true should not match lowercase àb on binary input, but should match it on utf8 input.
|
|
||
| explicit MatchLikeConstants(bool is_utf8) | ||
| : regex_options(MakeRE2Options(is_utf8)), | ||
| like_pattern_is_substring_match(R"(%+([^%_]*[^\\%_])?%+)", regex_options), |
There was a problem hiding this comment.
Optional readability nit: could we use default member initializers for these RE2 members so each regex literal stays next to the comment explaining it? The comments and regexes are tightly coupled because each one classifies a LIKE pattern that can be rewritten as substring / prefix / suffix search. Keeping them together would make these hard-to-read regexes easier to audit.
| RE2 like_pattern_is_starts_with; | ||
| // A LIKE pattern matching this regex can be translated into a suffix search. | ||
| RE2 like_pattern_is_ends_with; | ||
| Status init_status; |
There was a problem hiding this comment.
Optional nit: could init_status be a local in Make() instead of a MatchLikeConstants member? It appears to be construction-only state, so narrowing its scope would make the struct state tighter.
Rationale for this change
Some string kernels are generated twice for the corresponding binary and string type (for example binary/utf8, large_binary, large_utf8). Most of the time we can cut down on the code duplication to reduce compile times and binary size slightly.
What changes are included in this PR?
Only instantiate string kernels for base binary types, not the string derived types.
This makes
libarrow_computesomehow smaller on my system:Are these changes tested?
Yes, including additional tests to check that utf8-ness of the input is still propagated correctly.
Are there any user-facing changes?
No.