[C++] ChunkedArrayResolver::Resolve is prematurely template-specialized #40280
Comments
felipecrv
added a commit
that referenced
this issue
Feb 29, 2024
…es instead of entire class (#40281) ### Rationale for this change Less template-specialization without any loss in efficiency. ### What changes are included in this PR? - `ChunkedResolver` tweak - Explicitly declare copy constructors of `ChunkedResolver` - at the moment they are necessary because `ChunkedArrayResolver` is copied in compute kernel code - Make `ChunkedArrayResolver` re-use `ChunkResolver` via composition instead of inheritance - This will allow the `ChunkResolver` API to evolve in ways that might not make sense if it's a sub-class of `ChunkedArrayResolver` - Use `std::enable_if` instead of duplicating the `ResolvedChunk` implementation and template-specializing - Only specialize `ResolvedChunk::Value` on value-type-specific types preserving the same type-safety checks (static and runtime) ### Are these changes tested? Yes, by existing tests. ### Are there any user-facing changes? No, because these are API are under the internal namespace at the moment even though there are plans to make it public. These changes are preparation for that if we end up making them public. * GitHub Issue: #40280 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
|
Issue resolved by pull request 40281 |
thisisnic
pushed a commit
to thisisnic/arrow
that referenced
this issue
Mar 8, 2024
…ic types instead of entire class (apache#40281) ### Rationale for this change Less template-specialization without any loss in efficiency. ### What changes are included in this PR? - `ChunkedResolver` tweak - Explicitly declare copy constructors of `ChunkedResolver` - at the moment they are necessary because `ChunkedArrayResolver` is copied in compute kernel code - Make `ChunkedArrayResolver` re-use `ChunkResolver` via composition instead of inheritance - This will allow the `ChunkResolver` API to evolve in ways that might not make sense if it's a sub-class of `ChunkedArrayResolver` - Use `std::enable_if` instead of duplicating the `ResolvedChunk` implementation and template-specializing - Only specialize `ResolvedChunk::Value` on value-type-specific types preserving the same type-safety checks (static and runtime) ### Are these changes tested? Yes, by existing tests. ### Are there any user-facing changes? No, because these are API are under the internal namespace at the moment even though there are plans to make it public. These changes are preparation for that if we end up making them public. * GitHub Issue: apache#40280 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the enhancement requested
ChunkedArrayResolver::Resolve<T>can be made more generic by always producing aResolvedChunk<arrow::Array>instead ofResolvedChunk<arrow::Int64Array>ResolvedChunk<arrow::BoolArray>ResolvedChunk<arrow::StringArrayIt's only
ResolvedChunk::Valuethat needs to specialize on the sub-type ofarrow::Array.Other possible improvements that might end up with their own dedicated issues:
ChunkedArrayResolverconstructable fromstd::vectorsof other chunk array types (egstd::shared_ptr<Arrow>,std::shared_ptr<ArrayData>instead of justconst ArrayComponent(s)
C++
The text was updated successfully, but these errors were encountered: