You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2024/06/05 00:36:21 UTC
[PR] GH-41970: [C++] Misc changes making code around list-like types and list-view types behave the same way [arrow]
felipecrv opened a new pull request, #41971:
URL: https://github.com/apache/arrow/pull/41971
### Rationale for this change
These changes facilitate the refactoring of compute kernels and tests to support not only the list-like types but also list-views.
### What changes are included in this PR?
- Inclusion of `FixedSizeListBuilder::TypeClass` to make it in line with `VarLengthListLikeBuilder<TYPE>` classes
- Small documentation updates related to list-view types
- Change `list`/`large_list`/`fixed_size_list` factories to take `shared_ptr` by value instead of `const &`
- Change `ListType`/`LargeListType`/`FixedSizeList` constructors take `shared_ptr` by value instead of `const &`
### Are these changes tested?
Yes. By existing tests.
### Are there any user-facing changes?
- The `list`/`large_list`/`fixed_size_list` type factories now take the `shared_ptr` parameter value instead of `const &`.
- The `ListType`/`LargeListType`/`FixedSizeList` constructors now take the `shared_ptr` parameter by value instead of `const &`
<!-- **This PR includes breaking changes to public APIs.** -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41970: [C++] Misc changes making code around list-like types and list-view types behave the same way [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #41971:
URL: https://github.com/apache/arrow/pull/41971#discussion_r1629406589
##########
cpp/src/arrow/type_fwd.h:
##########
@@ -525,19 +525,19 @@ std::shared_ptr<DataType> decimal256(int32_t precision, int32_t scale);
/// \brief Create a ListType instance from its child Field type
ARROW_EXPORT
-std::shared_ptr<DataType> list(const std::shared_ptr<Field>& value_type);
+std::shared_ptr<DataType> list(std::shared_ptr<Field> value_type);
Review Comment:
We've already made such changes in the past and nobody complained. This doesn't morally change the API (any existing usage should still work properly) and we don't make any promises about the ABI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41970: [C++] Misc changes making code around list-like types and list-view types behave the same way [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41971:
URL: https://github.com/apache/arrow/pull/41971#discussion_r1626779898
##########
cpp/src/arrow/type_fwd.h:
##########
@@ -525,19 +525,19 @@ std::shared_ptr<DataType> decimal256(int32_t precision, int32_t scale);
/// \brief Create a ListType instance from its child Field type
ARROW_EXPORT
-std::shared_ptr<DataType> list(const std::shared_ptr<Field>& value_type);
+std::shared_ptr<DataType> list(std::shared_ptr<Field> value_type);
Review Comment:
@bkietz @pitrou should we treat this as a breaking change? If merging this causes no issue on the next release, I'm going to be more aggressive refactoring signatures moving forward (making sure I don't introduce bugs causes by `std::move`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41970: [C++] Misc changes making code around list-like types and list-view types behave the same way [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41971:
URL: https://github.com/apache/arrow/pull/41971#discussion_r1626780547
##########
cpp/src/arrow/compute/kernels/vector_hash.cc:
##########
@@ -698,13 +698,12 @@ void AddHashKernels(VectorFunction* func, VectorKernel base, OutputType out_ty)
DCHECK_OK(func->AddKernel(base));
}
- // Example parametric types that we want to match only on Type::type
- auto parametric_types = {time32(TimeUnit::SECOND), time64(TimeUnit::MICRO),
- timestamp(TimeUnit::SECOND), duration(TimeUnit::SECOND),
- fixed_size_binary(0)};
- for (const auto& ty : parametric_types) {
- base.init = GetHashInit<Action>(ty->id());
- base.signature = KernelSignature::Make({ty->id()}, out_ty);
+ // Parametric types that we want matching to be depededent only on type id
+ auto parametric_types = {Type::TIME32, Type::TIME64, Type::TIMESTAMP, Type::DURATION,
+ Type::FIXED_SIZE_BINARY};
+ for (const auto& type_id : parametric_types) {
+ base.init = GetHashInit<Action>(type_id);
+ base.signature = KernelSignature::Make({type_id}, out_ty);
Review Comment:
I didn't mention this in the description, but it seemed like an easy win while I was reading this code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41970: [C++] Misc changes making code around list-like types and list-view types behave the same way [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #41971:
URL: https://github.com/apache/arrow/pull/41971#discussion_r1629413512
##########
docs/source/cpp/compute.rst:
##########
@@ -1433,14 +1435,20 @@ null input value is converted into a null output value.
* \(3) The list offsets are unchanged, the list values are cast from the
input value type to the output value type (if a conversion is
- available).
+ available). If the output type is (Large)ListView, then sizes are
+ derived from the offsets.
+
+* \(4) If output type is list-like, offsets might have to be rebuilt to be
+ sorted and spaced correctly. If output type is a list-view type, the offsets
Review Comment:
The values also need to be rewritten for the views to be contiguous, right?
##########
cpp/src/arrow/compute/kernels/vector_hash.cc:
##########
@@ -698,13 +698,12 @@ void AddHashKernels(VectorFunction* func, VectorKernel base, OutputType out_ty)
DCHECK_OK(func->AddKernel(base));
}
- // Example parametric types that we want to match only on Type::type
- auto parametric_types = {time32(TimeUnit::SECOND), time64(TimeUnit::MICRO),
- timestamp(TimeUnit::SECOND), duration(TimeUnit::SECOND),
- fixed_size_binary(0)};
- for (const auto& ty : parametric_types) {
- base.init = GetHashInit<Action>(ty->id());
- base.signature = KernelSignature::Make({ty->id()}, out_ty);
+ // Parametric types that we want matching to be depededent only on type id
+ auto parametric_types = {Type::TIME32, Type::TIME64, Type::TIMESTAMP, Type::DURATION,
+ Type::FIXED_SIZE_BINARY};
+ for (const auto& type_id : parametric_types) {
+ base.init = GetHashInit<Action>(type_id);
+ base.signature = KernelSignature::Make({type_id}, out_ty);
Review Comment:
Sounds good to me.
##########
cpp/src/arrow/compute/kernels/vector_hash.cc:
##########
@@ -698,13 +698,12 @@ void AddHashKernels(VectorFunction* func, VectorKernel base, OutputType out_ty)
DCHECK_OK(func->AddKernel(base));
}
- // Example parametric types that we want to match only on Type::type
- auto parametric_types = {time32(TimeUnit::SECOND), time64(TimeUnit::MICRO),
- timestamp(TimeUnit::SECOND), duration(TimeUnit::SECOND),
- fixed_size_binary(0)};
- for (const auto& ty : parametric_types) {
- base.init = GetHashInit<Action>(ty->id());
- base.signature = KernelSignature::Make({ty->id()}, out_ty);
+ // Parametric types that we want matching to be depededent only on type id
Review Comment:
```suggestion
// Parametric types that we want matching to be dependent only on type id
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41970: [C++] Misc changes making code around list-like types and list-view types behave the same way [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv merged PR #41971:
URL: https://github.com/apache/arrow/pull/41971
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41970: [C++] Misc changes making code around list-like types and list-view types behave the same way [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41971:
URL: https://github.com/apache/arrow/pull/41971#discussion_r1626778707
##########
cpp/src/arrow/type.cc:
##########
@@ -3149,20 +3149,20 @@ std::shared_ptr<DataType> time64(TimeUnit::type unit) {
return std::make_shared<Time64Type>(unit);
}
-std::shared_ptr<DataType> list(const std::shared_ptr<DataType>& value_type) {
- return std::make_shared<ListType>(value_type);
+std::shared_ptr<DataType> list(std::shared_ptr<DataType> value_type) {
+ return std::make_shared<ListType>(std::move(value_type));
}
-std::shared_ptr<DataType> list(const std::shared_ptr<Field>& value_field) {
- return std::make_shared<ListType>(value_field);
+std::shared_ptr<DataType> list(std::shared_ptr<Field> value_field) {
+ return std::make_shared<ListType>(std::move(value_field));
}
-std::shared_ptr<DataType> large_list(const std::shared_ptr<DataType>& value_type) {
- return std::make_shared<LargeListType>(value_type);
+std::shared_ptr<DataType> large_list(std::shared_ptr<DataType> value_type) {
+ return std::make_shared<LargeListType>(std::move(value_type));
}
-std::shared_ptr<DataType> large_list(const std::shared_ptr<Field>& value_field) {
- return std::make_shared<LargeListType>(value_field);
+std::shared_ptr<DataType> large_list(std::shared_ptr<Field> value_field) {
+ return std::make_shared<LargeListType>(std::move(value_field));
Review Comment:
@amoeba this is related to what we've been discussing about conventions on type signatures in C++.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41970: [C++] Misc changes making code around list-like types and list-view types behave the same way [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41971:
URL: https://github.com/apache/arrow/pull/41971#discussion_r1626780547
##########
cpp/src/arrow/compute/kernels/vector_hash.cc:
##########
@@ -698,13 +698,12 @@ void AddHashKernels(VectorFunction* func, VectorKernel base, OutputType out_ty)
DCHECK_OK(func->AddKernel(base));
}
- // Example parametric types that we want to match only on Type::type
- auto parametric_types = {time32(TimeUnit::SECOND), time64(TimeUnit::MICRO),
- timestamp(TimeUnit::SECOND), duration(TimeUnit::SECOND),
- fixed_size_binary(0)};
- for (const auto& ty : parametric_types) {
- base.init = GetHashInit<Action>(ty->id());
- base.signature = KernelSignature::Make({ty->id()}, out_ty);
+ // Parametric types that we want matching to be depededent only on type id
+ auto parametric_types = {Type::TIME32, Type::TIME64, Type::TIMESTAMP, Type::DURATION,
+ Type::FIXED_SIZE_BINARY};
+ for (const auto& type_id : parametric_types) {
+ base.init = GetHashInit<Action>(type_id);
+ base.signature = KernelSignature::Make({type_id}, out_ty);
Review Comment:
I didn't mention this in the description, but it seemed like an easy win while I was reading this code, so I threw it in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41970: [C++] Misc changes making code around list-like types and list-view types behave the same way [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41971:
URL: https://github.com/apache/arrow/pull/41971#issuecomment-2161007599
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 53c1fc9f19709aa63d87cd5424950f675ca93dc9.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/26083866863) has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41970: [C++] Misc changes making code around list-like types and list-view types behave the same way [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41971:
URL: https://github.com/apache/arrow/pull/41971#discussion_r1631437626
##########
docs/source/cpp/compute.rst:
##########
@@ -1433,14 +1435,20 @@ null input value is converted into a null output value.
* \(3) The list offsets are unchanged, the list values are cast from the
input value type to the output value type (if a conversion is
- available).
+ available). If the output type is (Large)ListView, then sizes are
+ derived from the offsets.
+
+* \(4) If output type is list-like, offsets might have to be rebuilt to be
+ sorted and spaced correctly. If output type is a list-view type, the offsets
Review Comment:
true. I will clarify.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org