GH-39798: [C++] Optimize Take for fixed-size types including nested fixed-size lists #41297

felipecrv · 2024-04-18T18:06:11Z

Rationale for this change

Introduce utilities for dealing with fixed-width types (including fixed-size lists of fixed-width types) generically. And use it for initial optimizations of Take and Filter.

What changes are included in this PR?

Introduce utilities for dealing with fixed-width types generically
Use faster Take kernel on small power-of-2 byte widths of fixed-width types
- from FSLTakeExec (including FSLs of FSBs)
- from FSBTakeExec (done before this PR)
~~Take on any fixed-width type~~ (as a separate issue [C++] Extract the kernel loops used for PrimitiveTakeExec and generalize to any fixed-width type #41301)
Use faster Filter kernel on both primitive and fixed-width types of any length
- from FSLFilterExec (including FSLs of FSBs)
- from FSBFilterExec (done before this PR)

Are these changes tested?

By existing and new tests.

Are there any user-facing changes?

Some functions added to the arrow::util namespace and documented inline.

GitHub Issue: [C++] Optimize Take and Filter on fixed-size list of primitives #39798

cpp/src/arrow/util/fixed_width_internal.h

…nulls to a FixedSizeListBuilder

…locatePrimitiveArrayData

bkietz

Thank you for working on this! My greatest concern is the number of header utilities introduced for handling fixed-width-like. It seems unnecessary to ensure that so many of these are inlined when they don't appear inside a hot loop.

cpp/src/arrow/util/fixed_width_internal.h

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

cpp/src/arrow/util/fixed_width_test_util.h

cpp/src/arrow/util/fixed_width_test.cc

felipecrv · 2024-05-02T18:56:54Z

This push was just a rebase. Fixes coming next.

felipecrv · 2024-05-02T21:48:15Z

@bkietz linker errors fixed.

felipecrv · 2024-05-03T01:01:20Z

CI failures are unrelated. I'm going to merge now, but more feedback can be posted here or on the follow-up PRs that use and extend this work.

conbench-apache-arrow · 2024-05-03T05:35:28Z

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

There was 1 benchmark result indicating a performance regression:

Commit Run on ursa-i9-9960x at 2024-05-03 01:10:02Z
- tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-08, scale_factor=1

The full Conbench report has more details. It also includes information about 41 possible false positives for unstable benchmarks that are known to sometimes produce them.

felipecrv · 2024-05-03T15:35:48Z

@ursabot please benchmark lang=R

ursabot · 2024-05-03T15:35:54Z

Benchmark runs are scheduled for commit 0034a04. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

conbench-apache-arrow · 2024-05-03T18:55:24Z

Thanks for your patience. Conbench analyzed the 6 benchmarking runs that have been run so far on PR commit 0034a04.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

pitrou

Sorry for the belated review here.

cpp/src/arrow/util/fixed_width_internal.h

pitrou · 2024-05-06T14:01:28Z

cpp/src/arrow/util/fixed_width_internal.h

+/// \brief Checks if the given array has a fixed-width type or if it's an array of
+/// fixed-size list that can be flattened to an array of fixed-width values.
+///
+/// Fixed-width types are the ones defined by the is_fixed_width() predicate in


In general, it's nice for docstrings to be precise and explanatory, but a 150-lines long docstring ensures that most readers won't read it in entirety. Can you try to distill it down a bit?

Isn't that what \brief is for? It's rather long because of the examples.

Without them, it's really hard to understand the intricacies of this concept.

\brief is for a one-liner introductory sentence, but if every non-trivial API function gets a 150-line docstring, then nobody will read them anymore (and, besides, even the source code will become impossible to navigate).

I will remove the comments from the /// and spread the explanation, but I think this concept is complicated enough that having a long comment in the source code is worth it.

Changed this in #41597.

cpp/src/arrow/util/fixed_width_internal.h

pitrou · 2024-05-06T14:04:51Z

cpp/src/arrow/util/fixed_width_internal.h

+inline bool IsFixedWidthLike(const ArraySpan& source, bool force_null_count,
+                             ExtraPred extra_predicate) {
+  const auto* type = source.type;
+  // BOOL is considered fixed-width if not nested under FIXED_SIZE_LIST.


This is a rather weird API design choice. Won't it confuse callers?

I started without BOOL then almost every IsFixedWidthLike check in the Take kernel had to have || type->id() == BOOL.

Additionally, excluding BOOL would put this function in conflict with is_fixed_width(). I wanted to preserve the property that the set of arrays that passes this predicate is a superset of the arrays that can pass is_fixed_width(type->id()). How could an array be [Is]FixedWidthLike if it's not even [is_]fixed_width()?

My "weird API design choice" comment was about the "if not nested under FSL" part :-)

API change in #41597.

cpp/src/arrow/util/fixed_width_internal.h

pitrou · 2024-05-06T14:07:19Z

cpp/src/arrow/util/fixed_width_internal.h

+///      is_fixed_width(*mutable_array->type) SHOULD be true
+/// \return The pointer to the fixed-width values of an array or NULLPTR
+///         if pre-conditions are not satisfied.
+ARROW_EXPORT const uint8_t* OffsetPointerOfFixedWidthValues(const ArraySpan& source);


Most if not all API functions operating on arrays take the offset into account, so adding "Offset" to the function name does not bring any additional information. FixedWidthValuesPointer would be shorter and more consistent with MutableFixedWidthValuesPointer below.

The inconsistency with MutableFixedWidthValuesPointer was on purpose because that one requires the input to have offset==0.

Most if not all API functions operating on arrays take the offset into account...

And the pointers they return can easily have the offset applied to them after the fact. I gave the Offset- prefix to this one so it works as a reminder to the caller that ptr + arr.offset won't work (it might by accident), but it's not correct for the general case. The actual offset is a complicated sum of offset products.

cpp/src/arrow/util/fixed_width_internal.h

cpp/src/arrow/util/fixed_width_test_util.h

felipecrv · 2024-05-06T20:22:39Z

@pitrou replied to your comments, please resolve the ones that you think are acceptable and discuss further the ones you still think need to be addressed.

Many questions revolve around the handling of BOOL and offsets. Do you think I should extend this to support BOOL as well? With all the extra complexity that would bring?

…t bit-sized types (BOOL) (#41597) ### Rationale for this change Post-merge feedback from #41297. ### What changes are included in this PR? - Supporting `BOOL` as both a top-level and nested in FSL types - Removing the long example from the docstring of `IsFixedWidthLike` These changes don't affect users because this header was added recently and not released. ### Are these changes tested? Yes, by existing and new test cases. * GitHub Issue: #41596 Authored-by: Felipe Oliveira Carvalho <[email protected]> Signed-off-by: Felipe Oliveira Carvalho <[email protected]>

…sted fixed-size lists (apache#41297) ### Rationale for this change Introduce utilities for dealing with fixed-width types (including fixed-size lists of fixed-width types) generically. And use it for initial optimizations of `Take` and `Filter`. ### What changes are included in this PR? - [x] Introduce utilities for dealing with fixed-width types generically - [x] Use faster `Take` kernel on small power-of-2 byte widths of fixed-width types - [x] from `FSLTakeExec` (including FSLs of FSBs) - [x] from `FSBTakeExec` (done before this PR) - [x] ~Take on any fixed-width type~ (as a separate issue apache#41301) - [x] Use faster `Filter` kernel on both primitive and fixed-width types of any length - [x] from `FSLFilterExec` (including FSLs of FSBs) - [x] from `FSBFilterExec` (done before this PR) ### Are these changes tested? By existing and new tests. ### Are there any user-facing changes? Some functions added to the `arrow::util` namespace and documented inline. * GitHub Issue: apache#39798 Authored-by: Felipe Oliveira Carvalho <[email protected]> Signed-off-by: Felipe Oliveira Carvalho <[email protected]>

…support bit-sized types (BOOL) (apache#41597) ### Rationale for this change Post-merge feedback from apache#41297. ### What changes are included in this PR? - Supporting `BOOL` as both a top-level and nested in FSL types - Removing the long example from the docstring of `IsFixedWidthLike` These changes don't affect users because this header was added recently and not released. ### Are these changes tested? Yes, by existing and new test cases. * GitHub Issue: apache#41596 Authored-by: Felipe Oliveira Carvalho <[email protected]> Signed-off-by: Felipe Oliveira Carvalho <[email protected]>

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Apr 18, 2024

felipecrv force-pushed the fixed_modulo_nesting branch from eebb793 to 54f295e Compare April 18, 2024 23:49

felipecrv marked this pull request as ready for review April 18, 2024 23:49

felipecrv requested a review from bkietz April 19, 2024 00:04

felipecrv force-pushed the fixed_modulo_nesting branch from 54f295e to eee89df Compare April 19, 2024 00:08

felipecrv mentioned this pull request Apr 19, 2024

[C++] Extract the kernel loops used for PrimitiveTakeExec and generalize to any fixed-width type #41301

Closed

3 tasks

vibhatha mentioned this pull request Apr 19, 2024

GH-41262: [Java][FlightSQL] Implement stateless prepared statements #41237

Merged

felipecrv force-pushed the fixed_modulo_nesting branch 2 times, most recently from c29cc3e to 5831f32 Compare April 24, 2024 15:02

benibus reviewed Apr 25, 2024

View reviewed changes

cpp/src/arrow/util/fixed_width_internal.h Outdated Show resolved Hide resolved

cpp/src/arrow/util/fixed_width_internal.h Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 25, 2024

felipecrv requested a review from pitrou May 2, 2024 14:57

felipecrv added 8 commits May 2, 2024 11:58

apacheGH-41188: [C++] Don't recursively produce nulls when appending …

66f1c23

…nulls to a FixedSizeListBuilder

util: Introduce the fixed-width modulo nesting type concept

5d70f7d

vector_selection: Use PreallocateFixedWidthArrayData and delete Preal…

633c181

…locatePrimitiveArrayData

vector_selection_test: Extract DoCheckTake

ccf92e0

Take: Use PrimitiveTakeExec for fixed-width (modulo nesting) arrays

0b18ac4

Filter: Use PrimitiveFilterExec for any fixed-width type

ab96924

Rename FixedWidth{ModuloNesting->Like} and change comments

772fbc5

Revert the changes in FixedSizeListBuilder and disable some test cases

b4f1b64

bkietz requested changes May 2, 2024

View reviewed changes

felipecrv force-pushed the fixed_modulo_nesting branch from 5831f32 to b4f1b64 Compare May 2, 2024 18:56

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 2, 2024

felipecrv added 2 commits May 2, 2024 16:08

fixed_width: Add more test cases to FixedSizeWidthIn* functions

030a1a5

fixed_width: Add exclude_dictionary=false parameter

0fc35d1

github-actions bot removed the awaiting change review Awaiting change review label May 2, 2024

github-actions bot added awaiting merge Awaiting merge awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels May 2, 2024

fixed_width: Fix linker errors

0034a04

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 2, 2024

felipecrv merged commit 9749d7d into apache:main May 3, 2024
34 of 36 checks passed

felipecrv removed the awaiting change review Awaiting change review label May 3, 2024

This was referenced May 3, 2024

[C++] Optimize Take and Filter on fixed-size list of primitives #39798

Closed

[C++] Take the improvements from Take to the implementation of Filter #41510

Open

felipecrv deleted the fixed_modulo_nesting branch May 3, 2024 15:18

apache deleted a comment from ursabot May 3, 2024

pitrou reviewed May 6, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels May 6, 2024

This was referenced May 8, 2024

[C++] Support bit-sized types (BOOL) in fixed_width_util.h APIs #41596

Closed

GH-41596: [C++] fixed_width_internal.h: Simplify docstring and support bit-sized types (BOOL) #41597

Merged

felipecrv mentioned this pull request Jun 4, 2024

[C++] Rename the fixed-width concept from fixed_width_internal.h to "generalized fixed-width" #41963

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-39798: [C++] Optimize Take for fixed-size types including nested fixed-size lists #41297

GH-39798: [C++] Optimize Take for fixed-size types including nested fixed-size lists #41297

felipecrv commented Apr 18, 2024 •

edited

Loading

bkietz left a comment

felipecrv commented May 2, 2024

felipecrv commented May 2, 2024

felipecrv commented May 3, 2024

conbench-apache-arrow bot commented May 3, 2024

felipecrv commented May 3, 2024

ursabot commented May 3, 2024

conbench-apache-arrow bot commented May 3, 2024

pitrou left a comment

pitrou May 6, 2024

felipecrv May 6, 2024

pitrou May 7, 2024

felipecrv May 7, 2024

felipecrv May 8, 2024

pitrou May 6, 2024

felipecrv May 6, 2024

pitrou May 7, 2024

felipecrv May 8, 2024

pitrou May 6, 2024

felipecrv May 6, 2024

felipecrv commented May 6, 2024

GH-39798: [C++] Optimize Take for fixed-size types including nested fixed-size lists #41297

GH-39798: [C++] Optimize Take for fixed-size types including nested fixed-size lists #41297

Conversation

felipecrv commented Apr 18, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

bkietz left a comment

Choose a reason for hiding this comment

felipecrv commented May 2, 2024

felipecrv commented May 2, 2024

felipecrv commented May 3, 2024

conbench-apache-arrow bot commented May 3, 2024

felipecrv commented May 3, 2024

ursabot commented May 3, 2024

conbench-apache-arrow bot commented May 3, 2024

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

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

felipecrv commented May 6, 2024

felipecrv commented Apr 18, 2024 •

edited

Loading