pitrou commented on code in PR #50479:
URL: https://github.com/apache/arrow/pull/50479#discussion_r3636649298
##########
cpp/src/arrow/compute/kernels/scalar_string_ascii.cc:
##########
@@ -1337,27 +1343,47 @@ struct RegexSubstringMatcher {
template <typename Type, typename Matcher>
struct MatchSubstringImpl {
- using offset_type = typename Type::offset_type;
-
static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out,
const Matcher* matcher) {
- StringBoolTransform<Type>(
- ctx, batch,
- [&matcher](const void* raw_offsets, const uint8_t* data, int64_t
length,
- int64_t output_offset, uint8_t* output) {
- const offset_type* offsets = reinterpret_cast<const
offset_type*>(raw_offsets);
- FirstTimeBitmapWriter bitmap_writer(output, output_offset, length);
- for (int64_t i = 0; i < length; ++i) {
- const char* current_data = reinterpret_cast<const char*>(data +
offsets[i]);
- int64_t current_length = offsets[i + 1] - offsets[i];
- if (matcher->Match(std::string_view(current_data,
current_length))) {
+ if constexpr (is_binary_view_like_type<Type>::value) {
+ // Views have no packed offset buffer, so evaluate per element. Null
slots are
+ // skipped: a view's null header is not validated and may carry a bogus
+ // buffer_index/offset that decoding would dereference.
+ const ArraySpan& input = batch[0].array;
+ ArraySpan* out_arr = out->array_span_mutable();
+ FirstTimeBitmapWriter bitmap_writer(out_arr->buffers[1].data,
out_arr->offset,
+ input.length);
+ VisitArrayValuesInline<Type>(
Review Comment:
Note: I think ideally we would also use `VisitArrayValuesInline` for other
binary types, because the matcher may be expensive and it's better to skip it
on null entries.
(for example, if the array has 99% nulls, it would be wasteful to run a
regexp matcher on each underlying empty slot)
##########
cpp/src/arrow/compute/kernels/scalar_string_benchmark.cc:
##########
@@ -77,6 +87,28 @@ static void MatchSubstring(benchmark::State& state) {
UnaryStringBenchmark(state, "match_substring", &options);
}
+// Predicate run directly on a string_view array (the path added by this PR).
+static void MatchSubstringView(benchmark::State& state) {
+ MatchSubstringOptions options("abac");
+ UnaryStringBenchmark(state, "match_substring", &options, utf8_view());
+}
+
+// Baseline: the pre-PR workaround of casting the view column to utf8 first.
The
+// gap vs. MatchSubstringView is the cast that direct view support avoids.
+static void MatchSubstringViewCast(benchmark::State& state) {
Review Comment:
We don't care about benchmarking this IMHO. It was useful to judge this PR's
usefulness, but it's not useful for measuring Arrow C++ performance.
##########
cpp/src/arrow/compute/kernels/scalar_string_ascii.cc:
##########
@@ -1611,6 +1637,19 @@ const FunctionDoc match_like_doc(
{"strings"}, "MatchSubstringOptions", /*options_required=*/true);
#endif
+// Register the view kernels for a match-substring-style predicate. Registered
per
+// view type (not via GenerateVarBinaryViewBase) so utf8_view ->
StringViewType keeps
+// the is_utf8 distinction used by ignore_case/regex folding.
Review Comment:
I've opened https://github.com/apache/arrow/issues/50615 as it's frankly a
pity to duplicate kernel code just for the is_utf8 flag.
--
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]
