GH-35765: [C++] Split vector_selection.cc into more compilation units #35751

felipecrv · 2023-05-24T21:47:31Z

Rationale for this change

When working on #35001 I had a hard time figuring where to place the code for all possible combinations of filters and REE data. vector_selection.cc is hard to follow with so many kernels implemented in a single file. This PR splits the two biggest ones: filter and take. Stuff that can be shared by both stays is in vector_selection_internal.cc and vector_selection.cc is concerned with the registering of the functions and a few smaller kernels.

What changes are included in this PR?

vector_selection_(internal|take|filter).(cc|h) source files were extracted from vector_selection.cc

Are these changes tested?

Yes, by existing tests.

Closes: [C++] Split vector_selection.cc into more compilation units #35765

…(cc|h)

…eriving Selection

…consistency

felipecrv · 2023-05-24T21:50:19Z

@pitrou @westonpace I tried to separate the code-moving commits from the ones that make changes to explain what's being done step-by-step.

pitrou

This PR is not minor in code changes, even though most of it is moving code around. Can you open a GH issue?

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

pitrou · 2023-05-25T09:20:43Z

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

+                               const FunctionOptions* default_options,
+                               FunctionRegistry* registry);
+
+Status PreallocateData(KernelContext* ctx, int64_t length, int bit_width,


The name PreallocateData is probably too general, since it's selection-specific?

Renamed to PreallocatePrimitiveArrayData. It's here because it's used by both take and filter on primitive arrays.

pitrou · 2023-05-25T09:22:30Z

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

@@ -58,2119 +60,22 @@ using internal::OptionalBitIndexer;
 namespace compute {
 namespace internal {

+using FilterState = OptionsWrapper<FilterOptions>;
+using TakeState = OptionsWrapper<TakeOptions>;
+
 int64_t GetFilterOutputSize(const ArraySpan& filter,
                            FilterOptions::NullSelectionBehavior null_selection) {


Is this function still useful, since it's a trivial wrapper around GetBitmapFilterOutputSize?

It becomes this in #35750.

int64_t GetFilterOutputSize(const ArraySpan& filter, FilterOptions::NullSelectionBehavior null_selection) { if (filter.type->id() == Type::BOOL) { return GetBitmapFilterOutputSize(filter, null_selection); } return GetREEFilterOutputSize(filter, null_selection);

I will move it completely to vector_selection_filter.cc now and remove it here, but I haven't decided yet if the REEx* filter kernels are going to be implemented in vector_selection_filter.cc or in a separate .cc, so I might undo this later.

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

github-actions · 2023-05-25T14:01:08Z

Closes: [C++] Split vector_selection.cc into more compilation units #35765

assignUser · 2023-05-25T15:27:55Z

For reference: A minor PR touches <=2 files with no changes to behaviour. (e.g. fixing typos and similar things)

kou · 2023-05-25T21:05:21Z

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

+
+#include "arrow/array/data.h"
+#include "arrow/compute/api_vector.h"
+#include "arrow/compute/kernels/vector_selection_internal.h"


We should not include *internal.h from a non *internal.h headers because *internal.h aren't installed.

vector_selection_filter.h is also internal — included only by vector_selection.cc. Should I rename it with an internal suffix in the name?

But I just realized it's not needed. 😄 This is why I made PopulateFilterKernels take an output param at some point. That allows me to forward-declare SelectionKernelData and not include the full header.

As it's been done with vector_selection_take.h

IIRC the internal.h suffix is important as the cmake uses it to decide which headers get installed. So if it is actually a private/internal header it should be renamed imo. But @kou would know best!

IIRC the internal.h suffix is important as the cmake uses it to decide which headers get installed. So if it is actually a private/internal header it should be renamed imo.

Correct.

Makes sense. I'm also renaming vector_selection_(filter|take).(h|cc) and pushing now.

pitrou · 2023-05-30T15:07:44Z

@felipecrv Is this ready for review again?

felipecrv · 2023-05-30T15:15:47Z

@felipecrv Is this ready for review again?

Yes.

pitrou

+1, thanks for doing this @felipecrv

ursabot · 2023-05-31T01:16:47Z

Benchmark runs are scheduled for baseline = 431785f and contender = 44d1b61. 44d1b61 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️3.29% ⬆️0.56%] test-mac-arm
[Failed ⬇️0.32% ⬆️9.55%] ursa-i9-9960x
[Finished ⬇️0.91% ⬆️0.88%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 44d1b615 ec2-t3-xlarge-us-east-2
[Finished] 44d1b615 test-mac-arm
[Finished] 44d1b615 ursa-i9-9960x
[Finished] 44d1b615 ursa-thinkcentre-m75q
[Finished] 431785f3 ec2-t3-xlarge-us-east-2
[Failed] 431785f3 test-mac-arm
[Failed] 431785f3 ursa-i9-9960x
[Finished] 431785f3 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

felipecrv added 4 commits May 24, 2023 18:41

vector_selection.cc: Extract vector_selection_(internal|take|filter).…

dd7e1b4

…(cc|h)

vector_selection_internal.h: Add -Selection- to the name of classes d…

d89f8f0

…eriving Selection

vector_selection_internal.h: Move implementations from header to the .cc

6c259ca

vector_selection_(filter|take).cc: Add -Exec to all kernel impls for …

2eacf5f

…consistency

felipecrv requested a review from westonpace as a code owner May 24, 2023 21:47

github-actions bot added Component: C++ awaiting review Awaiting review labels May 24, 2023

felipecrv mentioned this pull request May 24, 2023

GH-35749: [C++] Handle run-end encoded filters in compute kernels #35750

Merged

3 tasks

Fix lint issue

c1ddcb8

pitrou requested changes May 25, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 25, 2023

felipecrv changed the title ~~MINOR: [C++] Split vector_selection.cc into more compilation units~~ GH-35765: [C++] Split vector_selection.cc into more compilation units May 25, 2023

Hide GetBitmapFilterOutputSize and GetTakeIndicesFromBitmap

2e013d5

felipecrv requested a review from pitrou May 25, 2023 17:04

felipecrv added 2 commits May 25, 2023 14:15

Rename Preallocate{->PrimitiveArray}Data

3bb1868

Remove old NOTE/TODO. Check is done in function.cc

5e3e42e

felipecrv force-pushed the vector_selection_split branch from 00a43f7 to 5e3e42e Compare May 25, 2023 17:16

kou reviewed May 25, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels May 25, 2023

Remove inclusion of vector_selection_internal.h from non-internal header

82f6d26

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels May 26, 2023

Add _internal suffix to internal .h and their .cc

68417e4

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 26, 2023

Revert removal of vector_selection_internal.h from others headers

c45658d

pitrou approved these changes May 30, 2023

View reviewed changes

pitrou merged commit 44d1b61 into apache:main May 30, 2023

felipecrv deleted the vector_selection_split branch May 30, 2023 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-35765: [C++] Split vector_selection.cc into more compilation units #35751

GH-35765: [C++] Split vector_selection.cc into more compilation units #35751

felipecrv commented May 24, 2023 •

edited by github-actions bot

Loading

felipecrv commented May 24, 2023

pitrou left a comment

pitrou May 25, 2023

felipecrv May 25, 2023

pitrou May 25, 2023

felipecrv May 25, 2023

github-actions bot commented May 25, 2023

assignUser commented May 25, 2023

kou May 25, 2023

felipecrv May 26, 2023

felipecrv May 26, 2023

felipecrv May 26, 2023

assignUser May 26, 2023

kou May 26, 2023

felipecrv May 26, 2023

pitrou commented May 30, 2023

felipecrv commented May 30, 2023

pitrou left a comment

ursabot commented May 31, 2023

GH-35765: [C++] Split vector_selection.cc into more compilation units #35751

GH-35765: [C++] Split vector_selection.cc into more compilation units #35751

Conversation

felipecrv commented May 24, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

felipecrv commented May 24, 2023

pitrou left a 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

github-actions bot commented May 25, 2023

assignUser commented May 25, 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

pitrou commented May 30, 2023

felipecrv commented May 30, 2023

pitrou left a comment

Choose a reason for hiding this comment

ursabot commented May 31, 2023

felipecrv commented May 24, 2023 •

edited by github-actions bot

Loading