GH-41418: [C++] Add [Large]ListView and Map nested types for scalar_if_else's kernel functions #41419
Conversation
|
|
|
cc @felipecrv PTAL this, thxs. |
| struct CaseWhenFunctor<Type, enable_if_var_size_list<Type>> { | ||
| struct CaseWhenFunctor< | ||
| Type, enable_if_t<is_base_list_type<Type>::value || is_list_view_type<Type>::value>> { |
There was a problem hiding this comment.
The list-view types should have their own specialization because they have a super-power that classic list types don't have: you can append the child values in any order and adjust the offset/size pairs of any random position to point to that area.
Feel free to copy and paste this class and enable_if it for is_list_view with a TODO(GH-<issue number>): a more efficient implementation for list-views is possible comment mentioning an issue you can create about this. Then we can tackle the optimization in a separate PR.
There was a problem hiding this comment.
you can append the child values in any order and adjust the offset/size pairs of any random position to point to that area.
Thanks for your suggestion, i will dig into the list-view types.
Feel free to copy and paste this class and enable_if it for
is_list_viewwith aTODO(GH-<issue number>): a more efficient implementation for list-views is possiblecomment mentioning an issue you can create about this. Then we can tackle the optimization in a separate PR.
Agree!
There was a problem hiding this comment.
- Optimization for list-view types in current thread will be tracked in [C++] A more efficient "case_when" specialization for list-view types #41453 for optimize
- The new commit includes case_when's list-view type benchmark for our performance regression in the future
github-actions
bot
added
awaiting changes
2. add benchmark for list-view types
github-actions
bot
added
awaiting change review
github-actions
bot
added
awaiting merge
|
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 0d7fac0. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 23 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…alar_if_else's kernel functions (apache#41419) ### Rationale for this change Add [Large]ListView and Map nested types for scalar_if_else's kernel functions ### What changes are included in this PR? 1. Add the list-view related types to `case_when`, `coalesce`'s kernel function and move the nested-types's added logic to a unified function for better management. 2. Add the `MapType` and related test for `if_else` ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#41418 Authored-by: ZhangHuiGui <[email protected]> Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
…alar_if_else's kernel functions (apache#41419) ### Rationale for this change Add [Large]ListView and Map nested types for scalar_if_else's kernel functions ### What changes are included in this PR? 1. Add the list-view related types to `case_when`, `coalesce`'s kernel function and move the nested-types's added logic to a unified function for better management. 2. Add the `MapType` and related test for `if_else` ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#41418 Authored-by: ZhangHuiGui <[email protected]> Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
Rationale for this change
Add [Large]ListView and Map nested types for scalar_if_else's kernel functions
What changes are included in this PR?
case_when,coalesce's kernel function and move the nested-types's addedlogic to a unified function for better management.
MapTypeand related test forif_elseAre these changes tested?
Yes
Are there any user-facing changes?
No