GH-50478: [C++][Compute] Support string_view/binary_view in scalar string predicate kernels - #50479
GH-50478: [C++][Compute] Support string_view/binary_view in scalar string predicate kernels#50479fangchenli wants to merge 5 commits into
Conversation
|
|
4b94187 to
4e3a526
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends Arrow C++ scalar compute string predicate/measurement kernels to accept string_view/binary_view inputs so predicates can run directly on view arrays (avoiding a cast-and-copy to utf8/binary), and updates docs/tests/benchmarks accordingly.
Changes:
- Register view-type kernels for substring/like predicates, find/count, length, and classification predicates.
- Add unit tests covering view arrays (including slicing/chunking via the shared test harness) and add benchmarks comparing direct view evaluation vs cast-to-utf8 baseline.
- Update C++ compute documentation to reflect new input type support and output type behavior for view inputs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/source/cpp/compute.rst | Documents added StringView/BinaryView support for relevant predicate/measurement kernels. |
| cpp/src/arrow/compute/kernels/scalar_string_utf8.cc | Registers utf8_length support for utf8_view (Int32 output). |
| cpp/src/arrow/compute/kernels/scalar_string_internal.h | Adds utf8_view registration for unary string predicates (classification/is_*) via StringPredicateFunctor. |
| cpp/src/arrow/compute/kernels/scalar_string_test.cc | Adds TestStringViewPredicates.* coverage for view inputs across several kernels. |
| cpp/src/arrow/compute/kernels/scalar_string_benchmark.cc | Adds benchmarks for match_substring on utf8_view and cast-to-utf8 workaround; refactors shared dataset generation. |
| cpp/src/arrow/compute/kernels/scalar_string_ascii.cc | Implements/Registers view support for match-substring predicates and find/count/length kernels, including StringOffsetType for view outputs. |
…lar string predicate kernels Add STRING_VIEW/BINARY_VIEW support to the scalar string predicate and measurement kernels that read a string and emit a fixed-width type: match_substring(+regex), match_like, starts_with, ends_with, find_substring(+regex), count_substring(+regex), binary_length, utf8_length, string_is_ascii, and the ascii_is_*/utf8_is_* classifiers. The matcher/predicate logic already operates on std::string_view, so the change feeds view elements into the existing logic: MatchSubstringImpl gets a per-element ArrayIterator branch for the view layout (the packed-offset fast path stays for base binary/string), and the applicator-based kernels only need registration entries plus a StringOffsetType helper mapping views to Int32 output. Kernels that emit strings/lists are left for a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4e3a526 to
19583cc
Compare
| TEST(TestStringViewPredicates, MatchSubstringRegex) { | ||
| MatchSubstringOptions options{"a+"}; | ||
| const auto* input = R"(["", "cat", null, "aaa banana", "concatenation"])"; | ||
| for (const auto& ty : {binary_view(), utf8_view()}) { |
There was a problem hiding this comment.
Could we add one utf8_view case with ignore_case=true and non-ASCII input here (for example mirroring the existing MatchSubstringIgnoreCase coverage)?
The implementation intentionally registers utf8_view as StringViewType instead of using the generic BinaryViewType dispatch, so it would be good to have a test that would fail if we accidentally lost the UTF-8/Latin1 distinction. The current view-specific tests are mostly ASCII, so they wouldn't catch that regression.
There was a problem hiding this comment.
Good call. Added TestStringViewPredicates.MatchSubstringIgnoreCase in 8082d31: it mirrors the existing MatchSubstringIgnoreCase coverage with pattern "aé(", ignore_case=true, and input ["abc", "aEb", "baÉ(", "aé(", "ae(", "Aé("] → [false, false, true, true, false, true]. The uppercase É matching lowercase é only holds because utf8_view folds over the full Unicode range; if it were dispatched through the generic BinaryViewType path (ASCII/Latin1 folding only) that slot would come back false, so the test fails on that regression.
…g UTF-8/Latin1 distinction Add a match_substring ignore_case=true case on utf8_view with non-ASCII input (uppercase E-acute) mirroring the existing MatchSubstringIgnoreCase coverage. utf8_view is registered as StringViewType rather than the generic BinaryViewType dispatch, so case folding spans the full Unicode range; this test fails if that distinction is ever lost. Requested in review by zanmato1984. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| ASSERT_OK_AND_ASSIGN(Datum matched, CallFunction("match_substring", {arr}, &contains)); | ||
| AssertDatumsEqual(Datum(expected), matched); | ||
| ASSERT_OK_AND_ASSIGN(Datum ascii, CallFunction("string_is_ascii", {arr})); | ||
| AssertDatumsEqual(Datum(expected), ascii); |
There was a problem hiding this comment.
Optional test coverage nit: could we also exercise one fixed-width numeric-output kernel in NullSlotWithUnvalidatedHeader, e.g. find_substring or utf8_length? The current regression covers the two boolean-output paths, while several newly registered view kernels go through the applicator path. It looks null-safe too, but this would make the null-slot header invariant explicit across both paths.
There was a problem hiding this comment.
Done in 26453db. Extended NullSlotWithUnvalidatedHeader to also call find_substring (→ [0, null]) and utf8_length (→ [2, null]) on the same crafted ["ok", <0xFF null header>] array, so the null-slot header invariant is now asserted on the applicator (ScalarUnaryNotNull) path as well as the two boolean-output paths.
…eric-output kernel NullSlotWithUnvalidatedHeader covered only the two boolean-output view paths (match_substring, string_is_ascii). Add find_substring and utf8_length so the "do not decode a null slot's unvalidated header" invariant is asserted on the applicator (ScalarUnaryNotNull) path too. Requested in review by zanmato1984. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zanmato1984
left a comment
There was a problem hiding this comment.
+1. Thanks for working on this!
|
@pitrou do you want to take another look? |
| ArraySpan* out_arr = out->array_span_mutable(); | ||
| FirstTimeBitmapWriter bitmap_writer(out_arr->buffers[1].data, out_arr->offset, | ||
| input.length); | ||
| VisitArrayValuesInline<Type>( |
There was a problem hiding this 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)
| // 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) { |
There was a problem hiding this 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.
| // 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. |
There was a problem hiding this comment.
I've opened #50615 as it's frankly a pity to duplicate kernel code just for the is_utf8 flag.
Rationale for this change
The scalar string predicate/measurement kernels return
NotImplementedforstring_view/binary_viewinput, so evaluating a string predicate (e.g. aLIKEfilter) on a view array first requires casting the whole column toutf8/binary, a full copy. This closes that gap so the predicates rundirectly on view arrays.
Part of the view-kernel effort (umbrella #44336, tracking #39634); follows
grouper keys (#50224) and cast (#50166). Take/filter (#43010) is handled
separately by #50164.
What changes are included in this PR?
Adds
STRING_VIEW/BINARY_VIEWkernels for the input-view, fixed-output subset:match_substring(_regex),match_like,starts_with,ends_withfind_substring(_regex),count_substring(_regex)binary_length,utf8_lengthstring_is_ascii,ascii_is_*,utf8_is_*Implementation notes:
so their view paths (an
if constexprbranch inMatchSubstringImpland inStringPredicateFunctor) iterate withVisitArrayValuesInline, which skipsnull slots. This matters for correctness: a null slot's view header is not
validated (
ValidateBinaryViewskips nulls) and may carry a bogusbuffer_index/offset that decoding would dereference. The base binary/string
fast paths are unchanged.
find/count/length) already skip nulls, sothey only needed registration entries plus a small
StringOffsetTypehelpermapping the view types to
Int32output (a view element length is int32-sized).Kernels that emit strings/lists (
utf8_upper/lower/trim*,replace_substring,split_pattern,utf8_slice_codeunits, …) are out of scopefor a follow-up, since constructing view output is the harder half.
Are these changes tested?
Yes. New
TestStringViewPredicates.*cases inscalar_string_test.cccover empty,inlined (≤12 byte), out-of-line (>12 byte), sliced, and null values for both
binary_viewandutf8_view, asserting results match the plain string/binarysemantics.
A
MatchSubstringView/MatchSubstringViewCastbenchmark pair quantifiesrunning the predicate directly on a view array vs. the pre-PR cast-to-utf8
workaround. Release build, ~1M rows of avg-16-byte ASCII, mean of 3 reps:
MatchSubstring(utf8, direct)MatchSubstringView(utf8_view, direct, this PR)MatchSubstringViewCast(view → cast to utf8 → match, pre-PR workaround)Evaluating the predicate directly on the view array is ~1.34x faster than
casting to utf8 first, by skipping the full-column copy. Direct view evaluation
is ~14% slower than native utf8 (inherent to the view layout's per-element
decode) but avoids the cast entirely.
Are there any user-facing changes?
Yes. These compute functions now accept
string_view/binary_viewinput. Thecompute.rsttype-support tables are updated accordingly.