Repository navigation
GH-54668: [C++][Compute] Implement equality comparison between Utf8View and other string types - #55171
GH-54668: [C++][Compute] Implement equality comparison between Utf8View and other string types#55171Awesome06 wants to merge 5 commits into
Conversation
…Utf8View and other string types
### Rationale for this change
Currently, Arrow compute supports comparing `Utf8` with `LargeUtf8` and comparing `Utf8View` with `Utf8View`, but attempting to compare a `Utf8View` array/scalar with either `Utf8` or `LargeUtf8` fails with:
```text
ArrowNotImplementedError: Function 'equal' has no kernel matching input types (string, string_view)
```
In `scalar_compare.cc`, comparison operators (`equal`, `not_equal`, `less`, `greater`, etc.) enforce homogeneous symmetric kernel signatures (`{ty, ty}`) and rely on `DispatchBest` to resolve heterogeneous types via `CommonBinary()`. However, `CommonBinary()` did not previously recognize `Type::STRING_VIEW` or `Type::BINARY_VIEW`, returning `nullptr` and causing `DispatchBest` to fail.
When mixing string/binary view types with offset-based string/binary types, promoting to the view type (`utf8_view` or `binary_view`) is optimal:
1. `ZeroCopyCastExec` in `scalar_cast_string.cc` reuses the existing character data buffer directly as variadic buffer 0, only allocating 16-byte descriptors without copying string payload data. (Conversely, promoting to `LargeUtf8` would require a deep copy of all string data into contiguous buffers).
2. For scalar-array comparisons (`string_view_array == "literal"`), only the scalar literal is converted ($O(1)$ zero-allocation), leaving array memory completely untouched.
### What changes are included in this PR?
1. `cpp/src/arrow/compute/kernels/codegen_internal.cc`:
- Extended `CommonBinary()` to handle `Type::STRING_VIEW` and `Type::BINARY_VIEW`.
- Promotes mixed string types to `utf8_view()`, and mixed binary/fixed-size types to `binary_view()`.
2. `cpp/src/arrow/compute/kernels/scalar_compare_test.cc`:
- Added `CheckDispatchBest` assertions for view <-> non-view binary and string combinations (`utf8`, `large_utf8`, `utf8_view`, `binary`, `large_binary`, `binary_view`, `fixed_size_binary`).
- Added `TEST(TestBinaryViewCompareKernel, MixedStringTypes)` validating:
- Short strings (<= 12 bytes, testing inline descriptors)
- Long strings (> 12 bytes, testing out-of-line prefix + buffer lookup)
- Sliced arrays (`array.Slice(1, 2)`)
- Empty arrays (`[]`) and all-null arrays (`[null, null]`)
- Array-scalar and scalar-array operations across `equal`, `not_equal`, `less`, and `greater`.
3. `python/pyarrow/tests/test_compute.py`:
- Added `test_compare_mixed_string_types` parameterized across all 9 permutations of `string`, `large_string`, and `string_view` (including sliced and >12-byte strings).
- Added `test_compare_mixed_binary_types` parameterized across all 9 permutations of `binary`, `large_binary`, and `binary_view`.
### Are these changes tested?
Yes:
- C++ unit tests in `arrow-compute-scalar-math-test` (including `TestCompareKernel.DispatchBest` and `TestBinaryViewCompareKernel.MixedStringTypes`). All 594 tests in the test suite pass with 0 failures.
- PyArrow unit tests added to `python/pyarrow/tests/test_compute.py`.
### Are there any user-facing changes?
Yes. Users can now directly compare `Utf8View` / `BinaryView` columns against `Utf8`, `LargeUtf8`, `Binary`, and `LargeBinary` columns or scalars in expressions and compute kernels without explicit casting or `ArrowNotImplementedError`.
### Was AI used for this PR?
AI was used to diagnose the type promotion dispatch path, draft implementation code, and construct test cases under developer direction. The changes and tests were reviewed, compiled, and verified locally.
**PR code and description written by:**
- [X] Human
- [X] AI
**Reviewed before submission by:**
- [X] Human
- [X] AI
- [ ] Not reviewed
* GitHub Issue: apache#54668
|
|
github-actions
Bot
added
Component: C++
awaiting review
|
|
There was a problem hiding this comment.
Can we compare these types directly? Promotion copies the whole scalar string, even when the first byte differs.
I tried mixed kernels using ScalarBinary and borrowed std::string_view values. They reuse the existing comparison operators without conversion.
Local equality benchmarks on Apple ARM64. Times are microseconds per call:
| Input | Same type | This PR | Direct mixed |
|---|---|---|---|
| 8 B, equal scalars | 1.16 | 2.78 | 1.23 |
| 4 KiB, equal scalars | 1.26 | 2.99 | 1.29 |
| 1 MiB, equal scalars | 22.06 | 36.72 | 21.74 |
| 1 MiB, first byte differs | 1.17 | 15.88 | 1.19 |
| 131,072 × 8 B, equal arrays | 318.48 | 585.41 | 297.23 |
| 131,072 × 64 B, equal arrays | 440.54 | 564.70 | 402.25 |
| 4,096 × 4 KiB, equal arrays | 445.84 | 446.22 | 424.39 |
For 131,072 rows, direct comparison also saves 2 MiB of temporary view descriptors. Arrow-pool allocation drops from about 2.02 MiB to 16 KiB. Long array strings already share their payload, so this saves descriptors, not payload copies.
Tested f53968e against the local prototype. The comparison suite and 486 additional checks passed with the prototype.
We could add kernels for utf8/utf8_view and large_utf8/utf8_view, in both directions. When creating less/less_equal, reverse the mixed signature together with the arguments.
…en string and string_view Avoid promoting string scalars to string_view in CommonBinary during scalar comparisons. Instead, register direct bidirectional kernels for utf8/binary and their view variants using ScalarBinary to compare borrowed string views zero-copy. Also reverse the input signature in MakeFlippedCompare so asymmetric arguments dispatch correctly to flipped comparison kernels. * Closes: apache#55171
|
@dmitry-chirkov-dremio Good catch on both the scalar copying and the array view descriptor allocation overhead. I've updated the patch to compare mixed Verified local benchmark parity across both matching and early-differing strings, and added tests covering bidirectional mixed comparisons and asymmetric ordering in |
…lipped comparisons In MakeFlippedCompare, only construct a reversed KernelSignature when input types differ (in_types[0] != in_types[1]). This ensures symmetric kernels retain their original signature and attached MatchConstraints (such as DecimalsHaveSameScale), preventing raw unscaled integer comparisons on decimals with differing scales. Also add regression tests for bound expressions and scalar comparisons with different decimal scales.
|
I have updated I've also added regression tests covering |
…als and string view boundary
Extend TestIssue54668 to cover: 1) Negative decimal comparisons with differing scales, ensuring signed two's complement unscaled integers (-100 < -90) do not falsely return true for -1.00 < -9.0. 2) StringView 12-byte inline vs 13-byte out-of-line payload boundary transitions. 3) Shared 4-byte prefix matching with differing out-of-line payloads (prefix collision). 4) Bound expressions with literals on the LHS ('banana' < view_col) testing expression simplifier flipping.
|
|
…scalars, embedded nulls, and dictionary views Expand TestBinaryViewCompareKernel and TestIssue54668 with additional edge-case test coverage for cross-type comparisons: - Asymmetric slice start offsets between string and string_view arrays - Explicit null scalar instances against string_view arrays - Embedded null bytes (\0) in out-of-line (>12B) variadic buffer views - Symmetric dictionary-encoded string vs. utf8_view dispatch
Rationale for this change
Currently, Arrow compute supports comparing
Utf8withLargeUtf8and comparingUtf8ViewwithUtf8View, but attempting to compare aUtf8Viewarray/scalar with eitherUtf8orLargeUtf8fails with:Rather than promoting string types to views (which allocates 16-byte descriptors and copies scalar heap data), this change adds direct mixed-type comparison kernels between offset-based strings (
Utf8,LargeUtf8,Binary,LargeBinary) and view types (Utf8View,BinaryView).By reading the respective string and view array/scalar buffers in-place via
applicator::ScalarBinary, comparison operations execute with zero memory pool allocations beyond the output boolean bitmap and achieve performance parity with same-type comparisons.What changes are included in this PR?
cpp/src/arrow/compute/kernels/scalar_compare.cc:(utf8, utf8_view),(large_utf8, utf8_view),(binary, binary_view), and(large_binary, binary_view)in both directions usingapplicator::ScalarBinary.MakeFlippedCompareto only rebuild kernel signatures when input argument types differ (in_types[0] != in_types[1]). This preserves original signature constraints (such asDecimalsHaveSameScale) on symmetric kernels while correctly reversing asymmetric signatures forlessandless_equal.DispatchExactImplfast-path inCompareFunction::DispatchBestafter dictionary decoding so dictionary-encoded strings compared with views match direct mixed kernels immediately.cpp/src/arrow/compute/kernels/scalar_compare_test.cc:equal,not_equal,less,less_equal,greater,greater_equal).less,less_equal,greater, andgreater_equal.cpp/src/arrow/compute/expression_test.cc:TestIssue54668DecimalComparisonDifferentScalesverifying bound expressions for<and<=with mismatched decimal scales (1.00 < 9.0)."banana" < view_col) testing expression simplifier guarantee flipping.python/pyarrow/tests/test_compute.py:test_compare_mixed_string_typesandtest_compare_mixed_binary_typesparameterized across all 9 permutations of string/binary types (including arrays, scalars, slices, and out-of-line strings).Are these changes tested?
Yes:
cpp/src/arrow/compute/kernels/scalar_compare_test.ccandcpp/src/arrow/compute/expression_test.cc.python/pyarrow/tests/test_compute.py.Are there any user-facing changes?
Yes. Users can now directly compare
Utf8ViewandBinaryViewcolumns againstUtf8,LargeUtf8,Binary, andLargeBinarycolumns and scalars across all comparison functions (equal,not_equal,less,less_equal,greater,greater_equal) without explicit casting.Was AI used for this PR?
AI was used to diagnose kernel dispatch paths, draft implementation code, and construct test cases under developer direction. The changes and tests were reviewed, compiled, and verified locally.
PR code and description written by:
Reviewed before submission by: