GH-36905: [C++] Add support for SparseUnion to selection functions #36906

js8544 · 2023-07-27T08:50:48Z

Rationale for this change

Dense unions are already supported in Take, Filter and DropNull but sparse ones are not.

What changes are included in this PR?

Add kernels for sparse unions to those functions.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes: [C++] Add support for SparseUnion to selection functions #36905

github-actions · 2023-07-27T08:51:21Z

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

pitrou · 2023-08-09T09:03:26Z

It seems that it reuses the DenseUnion approach, but it would be more efficient to reuse the Struct approach. What do you think?

js8544 · 2023-08-09T12:47:05Z

It seems that it reuses the DenseUnion approach, but it would be more efficient to reuse the Struct approach. What do you think?

Right, I've changed it to the struct approach. But there is room for improvement for SparseUnion: the unselect children can have any value, so we don't have to call take with the same indices for every child. I've left a TODO comment in the code for this.

pitrou · 2023-08-09T14:22:48Z

But there is room for improvement for SparseUnion: the unselect children can have any value, so we don't have to call take with the same indices for every child.

We don't, but would it improve anything to use different indices for each child?

pitrou · 2023-08-09T14:25:03Z

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

+          int8_t child_id = typed_values.child_id(index);
+          child_id_buffer_builder_.UnsafeAppend(type_codes_[child_id]);


This seems to be doing a pointless back-and-forth between type codes and child ids?

Suggested change

int8_t child_id = typed_values.child_id(index);

child_id_buffer_builder_.UnsafeAppend(type_codes_[child_id]);

child_id_buffer_builder_.UnsafeAppend(typed_values.type_code(index));

pitrou · 2023-08-09T14:32:26Z

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

@@ -863,6 +920,22 @@ Status DenseUnionFilterExec(KernelContext* ctx, const ExecSpan& batch, ExecResul
  return FilterExec<DenseUnionSelectionImpl>(ctx, batch, out);
 }

+Status SparseUnionFilterExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {


Nit: move this into vector_filter_internal.cc along StructFilterExec? (can probably also share some code between them...)

Moved and extracted a FilterWithTakeExec.

pitrou · 2023-08-09T14:33:35Z

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

+                    .Value(&indices));
+
+  Datum result;
+  RETURN_NOT_OK(Take(batch[0].array.ToArrayData(), Datum(indices),


Perhaps we can call SparseUnionTakeExec directly instead of going through the function lookup and execution machinery again?

pitrou · 2023-08-09T14:34:32Z

docs/source/cpp/compute.rst

 +---------------+--------+--------------+--------------+--------------+-------------------------+-----------+
-| take          | Binary | Any          | Integer      | Input type 1 | :struct:`TakeOptions`   | \(1) \(4) |
+| take          | Binary | Any          | Integer      | Input type 1 | :struct:`TakeOptions`   | \(4)      |


Fixed, and the previous line was also wrong.

js8544 · 2023-08-09T14:36:07Z

We don't, but would it improve anything to use different indices for each child?

Judging from https://github.com/apache/arrow/blob/main/cpp/src/arrow/compute/kernels/vector_selection_take_internal.cc#L369, we can make the unneeded indices the same as the needed ones, so that accessing values_data[indices_data[position]] is more cache friendly. But I agree that this is very subtle.

pitrou · 2023-08-09T14:47:40Z

Yes, this is quite subtle. Generating the indices arrays would cost much more, so I'm not sure it would be beneficial at the end.

js8544 · 2023-08-09T15:03:45Z

Yes, this is quite subtle. Generating the indices arrays would cost much more, so I'm not sure it would be beneficial at the end.

OK I'll remove that comment.

- avoid copying type codes array - remove skipping of sliced tests on dense unions - add union-take test with null indices

pitrou

+1, thanks a lot for this @js8544

js8544 · 2023-08-10T12:42:28Z

Thanks for the improvements!

conbench-apache-arrow · 2023-08-11T17:56:58Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit ebcf7bc.

There were 2 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-08-10 15:59:43Z
- file-read (Python) with compression=uncompressed, dataset=fanniemae_2016Q4, file_type=feather, output_type=table
- file-read (Python) with compression=uncompressed, dataset=fanniemae_2016Q4, file_type=feather, output_type=dataframe

The full Conbench report has more details.

…ons (apache#36906) ### Rationale for this change Dense unions are already supported in Take, Filter and DropNull but sparse ones are not. ### What changes are included in this PR? Add kernels for sparse unions to those functions. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#36905 Lead-authored-by: Jin Shang <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

apacheGH-36905: selection functions support sparse union

cba22d6

github-actions bot added Component: C++ Component: Documentation awaiting review Awaiting review labels Jul 27, 2023

js8544 added 4 commits July 28, 2023 04:20

fix unsafe cast

2db10e7

use struct approach

851c25f

pr feedback

6262f79

remove commment

cddf9c8

pitrou requested changes Aug 9, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 9, 2023

js8544 requested a review from pitrou August 9, 2023 16:01

Last-minute improvements:

200fd5f

- avoid copying type codes array - remove skipping of sliced tests on dense unions - add union-take test with null indices

pitrou approved these changes Aug 10, 2023

View reviewed changes

pitrou merged commit ebcf7bc into apache:main Aug 10, 2023
33 of 35 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-36905: [C++] Add support for SparseUnion to selection functions #36906

GH-36905: [C++] Add support for SparseUnion to selection functions #36906

js8544 commented Jul 27, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Jul 27, 2023

pitrou commented Aug 9, 2023

js8544 commented Aug 9, 2023

pitrou commented Aug 9, 2023

pitrou Aug 9, 2023

js8544 Aug 9, 2023

pitrou Aug 9, 2023

js8544 Aug 9, 2023

pitrou Aug 9, 2023

js8544 Aug 9, 2023

pitrou Aug 9, 2023

js8544 Aug 9, 2023

js8544 commented Aug 9, 2023

pitrou commented Aug 9, 2023

js8544 commented Aug 9, 2023

pitrou left a comment

js8544 commented Aug 10, 2023

conbench-apache-arrow bot commented Aug 11, 2023

		int8_t child_id = typed_values.child_id(index);
		child_id_buffer_builder_.UnsafeAppend(type_codes_[child_id]);

	\| take \| Binary \| Any \| Integer \| Input type 1 \| :struct:`TakeOptions` \| \(4) \|
	\| take \| Binary \| Any \| Integer \| Input type 1 \| :struct:`TakeOptions` \| \(3) \|

GH-36905: [C++] Add support for SparseUnion to selection functions #36906

GH-36905: [C++] Add support for SparseUnion to selection functions #36906

Conversation

js8544 commented Jul 27, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jul 27, 2023

pitrou commented Aug 9, 2023

js8544 commented Aug 9, 2023

pitrou commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js8544 commented Aug 9, 2023

pitrou commented Aug 9, 2023

js8544 commented Aug 9, 2023

pitrou left a comment

Choose a reason for hiding this comment

js8544 commented Aug 10, 2023

conbench-apache-arrow bot commented Aug 11, 2023

js8544 commented Jul 27, 2023 •

edited by github-actions bot

Loading