[C++] TakeCC is doing indices.num_chunks() Concatenate() calls when it could be doing only one #40207
Comments
felipecrv
changed the title
pitrou
pushed a commit
that referenced
this issue
Feb 28, 2024
…instead of TakeCA (#40206) ### Rationale for this change `take` concatenates chunks when it's applied to a chunked `values` array, but when the `indices` arrays is also `chunked` it concatenates `values` more than once -- one `Concatenate` call with `values.chunks()` for every chunk in `indices`. This PR doesn't remove the concatenation, but ensures it's done only once instead of `indices.size()` times. ### What changes are included in this PR? - Adding return type to the `TakeXX` names (-> `TakeXXY`) to makes code easier to understand - Adding benchmarks to `TakeCCC` — copied from #13857 - Remove the concatenation from the loop body (!) ### Are these changes tested? By existing tests. ### Are there any user-facing changes? A faster compute kernel. * GitHub Issue: #40207 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
|
Issue resolved by pull request 40206 |
zanmato1984
pushed a commit
to zanmato1984/arrow
that referenced
this issue
Feb 28, 2024
…akeAA instead of TakeCA (apache#40206) ### Rationale for this change `take` concatenates chunks when it's applied to a chunked `values` array, but when the `indices` arrays is also `chunked` it concatenates `values` more than once -- one `Concatenate` call with `values.chunks()` for every chunk in `indices`. This PR doesn't remove the concatenation, but ensures it's done only once instead of `indices.size()` times. ### What changes are included in this PR? - Adding return type to the `TakeXX` names (-> `TakeXXY`) to makes code easier to understand - Adding benchmarks to `TakeCCC` — copied from apache#13857 - Remove the concatenation from the loop body (!) ### Are these changes tested? By existing tests. ### Are there any user-facing changes? A faster compute kernel. * GitHub Issue: apache#40207 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
felipecrv
changed the title
thisisnic
pushed a commit
to thisisnic/arrow
that referenced
this issue
Mar 8, 2024
…akeAA instead of TakeCA (apache#40206) ### Rationale for this change `take` concatenates chunks when it's applied to a chunked `values` array, but when the `indices` arrays is also `chunked` it concatenates `values` more than once -- one `Concatenate` call with `values.chunks()` for every chunk in `indices`. This PR doesn't remove the concatenation, but ensures it's done only once instead of `indices.size()` times. ### What changes are included in this PR? - Adding return type to the `TakeXX` names (-> `TakeXXY`) to makes code easier to understand - Adding benchmarks to `TakeCCC` — copied from apache#13857 - Remove the concatenation from the loop body (!) ### Are these changes tested? By existing tests. ### Are there any user-facing changes? A faster compute kernel. * GitHub Issue: apache#40207 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug, including details regarding any error messages, version, and platform.
It's a performance bug.
Component(s)
C++
The text was updated successfully, but these errors were encountered: