Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-39565: [C++] Do not concatenate chunked values of fixed-width types to run "array_take" #41700

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from
Draft

GH-39565: [C++] Do not concatenate chunked values of fixed-width types to run "array_take" #41700

wants to merge 28 commits into from

Conversation

felipecrv
Copy link
Contributor

@felipecrv felipecrv commented May 17, 2024 •
edited

Rationale for this change

Concatenating a chunked array into a single array before running the array_take kernels is very inefficient and can lead to out-of-memory crashes. See also #25822.

What changes are included in this PR?

  • Improvements in the dispatching logic of TakeMetaFunction("take") to make "array_take" able to have a chunked_exec kernel for some types
  • Implementation of kernels for "array_take" that can receive a ChunkedArray as values and produce an output without concatenating these chunks

Are these changes tested?

By existing tests. Some tests were added in previous PRs that introduced some of the infrastructure to support this.

@felipecrv
apacheGH-41188: [C++] Don't recursively produce nulls when appending …
8e19368 
…nulls to a FixedSizeListBuilder
@felipecrv
Introduce the Gather class helper
46acd3a
@felipecrv
Take: Use Gather for PrimitiveTakeImpl
7f2aacd
@felipecrv
Take: Pass bit-width instead of byte-width
66a83ac
@felipecrv
Take: Use a Gather specialization for boolean arrays
26e7740
@felipecrv
Take: Skip allocating validity bitmap when values and filter don't ha…
cdb1c05 
…ve nulls
@felipecrv
Take: Fold the implementation for FSB arrays into the one used for pr…
f3839e8 
…imitives
@felipecrv
Revert "apacheGH-41188: [C++] Don't recursively produce nulls when ap…
bdc3ed7 
…pending nulls to a FixedSizeListBuilder"

This reverts commit b31590c.
@felipecrv
gather_internal.h: Please the linter
30fac2d
@felipecrv
macros.h: Fix ARROW_FORCE_INLINE for MSVC
0e5a97a
@felipecrv
gather_internal.h: Small fixup to top-level comment
13ee2ca
@felipecrv
gather_internal.h: Rename {->k}WithFactor and use static_assert
ed38b80
@felipecrv
gather_internal.h: Use int64_t instead of size_t for size factors
77d292b
@felipecrv
gather_internal.h: Fix assertion: src_validity.offset is not always r…
c2dc17a 
…elated to src_offset_
@felipecrv
TakeCAC: Extract TakeAAC for simpler delegation
146b4f1
@felipecrv
Take: Pass ChunkedArray as shared_ptr& in TakeXXX functions
b51555b
@felipecrv
Take: Move all TakeXXX() functions to the TakeMetaFunction class
32b9eb6
@felipecrv
Take: Review the TakeXXX delegation logic
b65c807
@felipecrv
Take: Add TakeCAA, and remove TakeAAC
6a32a8a 
TakeCAA will be used from TakeCCC soon.
@felipecrv
Take: Add VectorKernel::ChunkedExec to SelectionKernelData
06d2ba9
@felipecrv
Take: VectorKernel::output_chunked should be false for "array_take"
0aa831b
@felipecrv
Take: Make "array_take" handle CA->A cases by populating VectorKernel…
ade4c17 
…::chunked_exec

Before this commit, only the "take" meta function could handle CA
parameters.
@felipecrv
Take: Create SpecialTakeChunkedExecFunctor to wrap CA->A kernels
519f04c
@felipecrv
gather_internal.h: Introduce GatherFromChunks
3626ab6
@felipecrv
Take: Change FixedWidthTakeImpl::Exec return type from void to Status
cae5d87
@felipecrv
Take: Introduce ValueSpan to delay dispatching on chunked-ness
9cbadcd
@felipecrv
Take: Implement the FixedWidthTakeChunkedExec() kernel using GatherFr…
022f7f3 
…omChunks
@felipecrv felipecrv changed the title GH-39565: [C++] GH-39565: [C++] Do not concatenate ChunkedArray values to run "array_take" May 17, 2024
@felipecrv felipecrv changed the title GH-39565: [C++] Do not concatenate ChunkedArray values to run "array_take" GH-39565: [C++] Do not concatenate chunked values of fixed-width types to run "array_take" May 17, 2024
@felipecrv felipecrv mentioned this pull request May 17, 2024
Draft
felipecrv
felipecrv commented May 17, 2024
View reviewed changes
cpp/src/arrow/compute/kernels/vector_selection_take_internal.cc
Comment on lines +794 to +795
// XXX: this loop can use TakeCAA once it can handle ChunkedArray
// without concatenating first
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on this one.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels May 17, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 17, 2024
@mapleFU
Copy link
Member

mapleFU commented May 17, 2024

May I ask a unrelated question, when would we call assert and when call DCHECK, since I think they would likely to be same?

@felipecrv
Copy link
Contributor Author

May I ask a unrelated question, when would we call assert and when call DCHECK, since I think they would likely to be same?

We call assert in headers because we don't want to pay the cost of including logging.h everywhere. Think of assert as lighter-weight debug checks. But if you see an assert in a .cc file tell me to change it to DCHECK*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
awaiting change review Awaiting change review Component: C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@felipecrv @mapleFU