GH-45167: [C++] Implement Compute Equals for List Types #45272

WillAyd · 2025-01-15T16:38:51Z

Rationale for this change

While equality exists for ListScalars, it is not available through the compute module. This makes that now possible.

What changes are included in this PR?

I have added equals and not_equals support to the compute module for list types

Are these changes tested?

Yes - see added changes

Are there any user-facing changes?

Yes - the new feature to allow list comparison through the compute module

GitHub Issue: [C++][Python] Implement pc.equal for List arguments #45167

github-actions · 2025-01-15T16:39:19Z

⚠️ GitHub issue #45167 has been automatically assigned in GitHub to PR creator.

cpp/src/arrow/compute/kernels/codegen_internal.h

WillAyd · 2025-01-21T20:46:33Z

cpp/src/arrow/compute/kernels/codegen_internal.h

+  explicit ArrayIterator(const ArraySpan& arr) : arr(arr), position(0) {}
+
+  T operator()() {
+    const auto array_ptr = arr.ToArray();


The alternative to calling ToArray with the cast would be to implement something like value_slice on the ArraySpan directly, although I'm not sure if the ArraySpan is supposed to return anything but pointers to primitives (as is currently implemeted)

This is going to be slow, so we probably want to avoid this IMHO.

You may want to run a crude benchmark from Python to check this.

How about:

Get offset, length

Subslice the value array

Build ListScalar / LargeListScalar from the child array?

Or materialize the child array, and using the sub array. arr.ToArray() every call is too expansive?

Sorry for the late reply - I somehow missed this comment. So the expectationn is that the ArraySpan -> Array conversion is expensive here and getting a slice first before making that conversion should help with performance, right?

The conversion will be expensive regardless of whether you slice before or afterwards, so ideally we should avoid the conversion entirely.

In other words, we should consider exposing a ArrayRangeEquals variant that takes ArraySpan inputs instead of Array (the underlying implementation can be converted to ArraySpan entirely).

WillAyd · 2025-01-23T14:15:47Z

@pitrou @jorisvandenbossche would either of you be able to take a look here?

pitrou · 2025-01-23T17:47:38Z

cpp/src/arrow/compute/kernels/scalar_compare.cc

@@ -445,6 +445,14 @@ std::shared_ptr<ScalarFunction> MakeCompareFunction(std::string name, FunctionDo
    DCHECK_OK(func->AddKernel({ty, ty}, boolean(), std::move(exec)));
  }

+  if constexpr (std::is_same_v<Op, Equal> || std::is_same_v<Op, NotEqual>) {
+    for (const auto id : {Type::LIST, Type::LARGE_LIST}) {
+      auto exec = GenerateList<applicator::ScalarBinaryEqualTypes, BooleanType, Op>(id);


Another approach with perhaps a better performance potential would be to leverage the existing RangeDataEqualsImpl in arrow/compare.cc.

Thanks for the heads up - I will give that a look. So I see all of the functions right now in the compare module are registered via RegisterScalarComparison. With what you are suggesting, I'm guessing I should be creating a new registry function along with that like RegisterRangeComparison right?

Also I'm guessing the RangeDataEqualsImpl is supposed to work when comparing two arrays, but not when comparing an array with a scalar

FWIW though I did benchmark the current implementation and it was definitely slow. Seemed about 1000x slower than an equivalent comparison using primitive types

With what you are suggesting, I'm guessing I should be creating a new registry function along with that like RegisterRangeComparison right?

I think we can avoid that by directly calling into RangeDataEqualsImpl.

Also I'm guessing the RangeDataEqualsImpl is supposed to work when comparing two arrays, but not when comparing an array with a scalar

A list scalar's value is actually an array, so that should not necessarily be a problem.

OK took a closer look at this. So AFAICT the RangeDataEqualsImpl returns a scalar bool value, rather than an array of booleans like we would need in the result here. That class is also private to the compare.cc module and doesn't expose any suitable entrypoint in compare.h that I think would work here.

Are you thinking we should refactor the RangeDataEqualsImpl to support vector functions and move it to make it accessible to the compute module, or do you think we should just create a dedicated class drawing some inspiration from it in scalar_compute.cc?

Thanks again for the guidance and patience here! Trying to wrap my head around the structure of the compute modules

So AFAICT the RangeDataEqualsImpl returns a scalar bool value, rather than an array of booleans like we would need in the result here.

That's right, so it would need to be called once for each list element (which is admittedly non optimal, but probably better than using GetScalar anyway?).

That class is also private to the compare.cc module and doesn't expose any suitable entrypoint in compare.h that I think would work here.

Well, we could add a suitable entrypoint in compare_internal.h if that's useful.

Another possible approach would be to leverage the comparison kernel for the child type, but that would probably be even more involved. So that's up to how much work you want to put into this :)

OK sorry for the long delay in getting back to this. I think I have a structure working but I'm unclear on if there's a bug with the RangeDataEqualsImpl implementation or if I am misunderstanding its purpose.

As an example, I am trying to compare two list arrays, where the first is:

[4, 5, 6]

and the second is:

[4, 5]

When calling RangeDataEqualsImpl::Compare with these two values, I am getting back true. The current Compare implementation looks like:

bool Compare() { // Compare null bitmaps if (left_start_idx_ == 0 && right_start_idx_ == 0 && range_length_ == left_.length && range_length_ == right_.length) { // If we're comparing entire arrays, we can first compare the cached null counts if (left_.GetNullCount() != right_.GetNullCount()) { return false; } } if (!OptionalBitmapEquals(left_.buffers[0], left_.offset + left_start_idx_, right_.buffers[0], right_.offset + right_start_idx_, range_length_)) { return false; } // Compare values return CompareWithType(*left_.type); }

So the first branch is entirely ignored because the length of the two ArrayData instances are not equal. Ultimately this falls down to the CompareWithType(*left_.type) call at the bottom of the method, but the ArrayData->type member reports back as INT32.

So is this a problem with the implementation of Compare where it should be short circuiting when the ArrayData lengths are not equal? And/or is the fact that the ArrayData->type member is coming back as INT32 and not as a List element the problem?

I see these comments at the beginning of the class:

class RangeDataEqualsImpl { public: // PRE-CONDITIONS: // - the types are equal // - the ranges are in bounds

We should probably add that the two ranges must have the same length.

For example this is how list comparison is currently implemented, where it is first checked that each pair of list elements have the same length:

template <typename offset_type, typename CompareRanges> void CompareWithOffsets(int offsets_buffer_index, CompareRanges&& compare_ranges) { const offset_type* left_offsets = left_.GetValues<offset_type>(offsets_buffer_index) + left_start_idx_; const offset_type* right_offsets = right_.GetValues<offset_type>(offsets_buffer_index) + right_start_idx_; const auto compare_runs = [&](int64_t i, int64_t length) { for (int64_t j = i; j < i + length; ++j) { if (left_offsets[j + 1] - left_offsets[j] != right_offsets[j + 1] - right_offsets[j]) { return false; } } if (!compare_ranges(left_offsets[i], right_offsets[i], left_offsets[i + length] - left_offsets[i])) { return false; } return true; }; VisitValidRuns(compare_runs); }

jorisvandenbossche · 2025-01-28T07:13:24Z

I currently have no time to review this in depth, but API-wise one remark: right now (for primitive arrays), nulls propagate in an operation like equal.
So how do they behave for nested typed, i.e. what if there is a null in a list element. Does that propagate as well (and does it make the comparison of the full list element null), or do we then consider a list element with nulls in the same location as equal?

WillAyd · 2025-01-28T14:42:55Z

The current (rather slow) implementation just does an elementwise compare, dispatching to the logical list scalar type. Therefore, since:

>>> l1 = pa.scalar([], type=pa.list_(pa.int32()))
>>> l2 = pa.scalar([], type=pa.list_(pa.int32()))
>>> l1 == l2
True

Wrapping that in an array does not change the behavior:

>>> arr1 = pa.array([l1])
>>> arr2 = pa.array([l2])
>>> pc.equal(arr1, arr2)
<pyarrow.lib.BooleanArray object at 0x71e8e9232560>
[
  true
]

mapleFU · 2025-02-06T09:36:43Z

cpp/src/arrow/compute/kernels/codegen_internal.h

@@ -390,6 +417,12 @@ struct UnboxScalar<Type, enable_if_has_string_view<Type>> {
  }
 };

+template <typename Type>
+struct UnboxScalar<Type, enable_if_list_type<Type>> {


So fixed_size_list is also declared as scalar, but not being registered in compute? ( It's ok to me, just to make sure this)

I can look at adding that to the compute registry

mapleFU · 2025-02-06T09:42:39Z

cpp/src/arrow/compute/kernels/codegen_internal.h

+  explicit ArrayIterator(const ArraySpan& arr) : arr(arr), position(0) {}
+
+  T operator()() {
+    const auto array_ptr = arr.ToArray();


How about:

Get offset, length

Subslice the value array

Build ListScalar / LargeListScalar from the child array?

Or materialize the child array, and using the sub array. arr.ToArray() every call is too expansive?

WillAyd · 2025-03-09T05:07:16Z

Sorry for the slow turnaround - hope this is heading in the right direction. Still nowhere close to the performance of more primitive types but about 2-3 times as fast as the previous implementation

>>> arr1 = pa.array([list("abc"), list("def"), list("xyz")] * 1_000_000)
>>> arr2 = pa.array([list("abc"), list("def"), list("xyzz")] * 1_000_000)
>>> %timeit pc.equal(arr1, arr2)
463 ms ± 5.33 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

WillAyd · 2025-07-02T13:35:31Z

OK I am still seeing some runtime performance after refactoring the RangeDataEqualsImpl class to compare_internal.h and creating custom functors that call that directly per element, although its still a good deal behind primitive performance:

>>> arr1 = pa.array([list("abc"), list("def"), list("xyz")] * 1_000_000)
>>> arr2 = pa.array([list("abc"), list("def"), list("xyzz")] * 1_000_000)
>>> %timeit pc.equal(arr1, arr2)
336 ms ± 1.19 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

I wonder if the ArrayIterator returning a shared_ptr<ArrayData> is the next bottleneck?

pitrou · 2025-07-02T13:49:34Z

I wonder if the ArrayIterator returning a shared_ptr is the next bottleneck?

Well, if we want an ArrayIterator for list arrays, it should not store a ArrayData but a ArraySpan.
This implies that RangeDataEqualsImpl should be modified to take ArraySpan as well.

(sorry, edited my message to remove an erroneous suggestion)

WillAyd · 2025-07-02T15:40:30Z

OK taking a look there. The main difference I noticed is that ArraySpan exposes its buffers class member as an BufferSpan array whereas ArrayData exposes it as a vector of shared pointers to a buffer. Additionally, the ArraySpan has a dictionary class method whereas ArrayData has a dictionary class member that is a shared pointer.

Do you think its worth trying to align the signatures of these elements to make templating easier, or should I perhaps use templating to disable the Visit methods in RangeDataEqualsImpl where those are currently being accessed?

pitrou · 2025-07-02T15:43:07Z

Ideally we shouldn't template the methods to accept both ArrayData and ArraySpan. It would be better (if possible) to migrate RangeDataEqualsImpl to use ArraySpan internally, and let the callers do the ArrayData -> ArraySpan conversion where required.

github-actions bot added Component: C++ awaiting review Awaiting review labels Jan 15, 2025

WillAyd commented Jan 15, 2025

View reviewed changes

cpp/src/arrow/compute/kernels/codegen_internal.h Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 15, 2025

WillAyd force-pushed the implement-list-compare branch from 573867e to 649e3b0 Compare January 16, 2025 20:09

WillAyd commented Jan 17, 2025

View reviewed changes

cpp/src/arrow/compute/kernels/codegen_internal.h Outdated Show resolved Hide resolved

WillAyd force-pushed the implement-list-compare branch from 649e3b0 to 68bb513 Compare January 21, 2025 20:43

WillAyd marked this pull request as ready for review January 21, 2025 20:44

WillAyd commented Jan 21, 2025

View reviewed changes

pitrou reviewed Jan 23, 2025

View reviewed changes

WillAyd force-pushed the implement-list-compare branch from 68bb513 to 3b055a8 Compare January 27, 2025 22:47

WillAyd force-pushed the implement-list-compare branch from 3b055a8 to ba3b114 Compare February 4, 2025 19:43

mapleFU reviewed Feb 6, 2025

View reviewed changes

WillAyd force-pushed the implement-list-compare branch 2 times, most recently from a41bbd0 to 970eb48 Compare March 9, 2025 05:05

WillAyd force-pushed the implement-list-compare branch from 970eb48 to 3071894 Compare March 10, 2025 14:59

WillAyd force-pushed the implement-list-compare branch from 3071894 to 364390c Compare May 4, 2025 15:57

WillAyd force-pushed the implement-list-compare branch from 89a547f to bd3bd92 Compare May 21, 2025 15:23

WillAyd added 6 commits June 30, 2025 09:25

apacheGH-45167: [C++] Implement Compute Equals for List Types

f764265

Slice child array

4ca0e9b

Working solution with Arrays

2899d2e

Use ArrayData for comparison

0e62ac8

Add const

eec2fbd

Fix rebase

66b52aa

WillAyd force-pushed the implement-list-compare branch from bd3bd92 to 66b52aa Compare June 30, 2025 14:20

WillAyd added 2 commits July 1, 2025 15:47

Move RangeDataEqualsimpl to internal header

afa8111

Update function registry to use RangeDataequalsimpl

9218e8e

GH-45167: [C++] Implement Compute Equals for List Types #45272

Are you sure you want to change the base?

GH-45167: [C++] Implement Compute Equals for List Types #45272

Uh oh!

Conversation

WillAyd commented Jan 15, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jan 15, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd commented Jan 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche commented Jan 28, 2025

Uh oh!

WillAyd commented Jan 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd commented Mar 9, 2025

Uh oh!

WillAyd commented Jul 2, 2025

Uh oh!

pitrou commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WillAyd commented Jul 2, 2025

Uh oh!

pitrou commented Jul 2, 2025

Uh oh!

Uh oh!

WillAyd commented Jan 15, 2025 •

edited by github-actions bot

Loading

pitrou Jul 2, 2025 •

edited

Loading

pitrou commented Jul 2, 2025 •

edited

Loading