zanmato1984 commented on code in PR #48459:
URL: https://github.com/apache/arrow/pull/48459#discussion_r2663471940
##########
cpp/src/arrow/type_traits.h:
##########
@@ -1837,4 +1838,23 @@ constexpr bool is_union(const DataType& type) { return
is_union(type.id()); }
/// @}
+/// \addtogroup c-type-concepts
Review Comment:
Shall we revert all the C++20 stuff in this PR and redo it in a separate
one? Or, we can define such concepts local in the cpp file, and only move them
to public header in a coming PR for #48590 ?
##########
cpp/src/arrow/compute/kernels/aggregate_test.cc:
##########
@@ -1980,6 +2001,7 @@ TYPED_TEST(TestFloatingMinMaxKernel, Floats) {
this->AssertMinMaxIs("[5, -Inf, 2, 3, 4]", -INFINITY, 5, options);
this->AssertMinMaxIsNull("[5, null, 2, 3, 4]", options);
this->AssertMinMaxIsNull("[5, -Inf, null, 3, 4]", options);
+ this->AssertMinMaxIsNull("[NaN, null]", options);
Review Comment:
Same as my other comment.
##########
cpp/src/arrow/acero/hash_aggregate_test.cc:
##########
@@ -2000,6 +2000,74 @@ TEST_P(GroupBy, MinMaxScalar) {
}
}
+TEST_P(GroupBy, MinMaxWithNaN) {
+ auto in_schema = schema({
+ field("argument1", float32()),
+ field("argument2", float64()),
+ field("key", int64()),
+ });
Review Comment:
The min/max kernel for half-float is not implemented so we are not able to
test it.
The current half-float is not intact in terms of: 1) whether the type is in
floating point category is inconsistent: e.g. type trait
`is_floating_type<HalfFloatType>` is `true` but `g_floating_types` doesn't
include it (that's why floating point kernels don't register for half-float).
2) Some `std` functions don't work with our half-float representation: e.g.
`std::is_nan/fmin/fmax`, it won't compile if we try to add certain half-float
kernels.
##########
cpp/src/arrow/compute/kernels/hash_aggregate.cc:
##########
@@ -276,46 +276,34 @@ struct AntiExtrema {
static constexpr CType anti_max() { return
std::numeric_limits<CType>::min(); }
};
-template <>
-struct AntiExtrema<bool> {
- static constexpr bool anti_min() { return true; }
- static constexpr bool anti_max() { return false; }
+template <CBooleanConcept CType>
+struct AntiExtrema<CType> {
+ static constexpr CType anti_min() { return true; }
+ static constexpr CType anti_max() { return false; }
};
-template <>
-struct AntiExtrema<float> {
- static constexpr float anti_min() { return
std::numeric_limits<float>::infinity(); }
- static constexpr float anti_max() { return
-std::numeric_limits<float>::infinity(); }
+template <CFloatingPointConcept CType>
+struct AntiExtrema<CType> {
+ static constexpr CType anti_min() { return
std::numeric_limits<CType>::quiet_NaN(); }
+ static constexpr CType anti_max() { return
std::numeric_limits<CType>::quiet_NaN(); }
};
-template <>
-struct AntiExtrema<double> {
- static constexpr double anti_min() { return
std::numeric_limits<double>::infinity(); }
- static constexpr double anti_max() { return
-std::numeric_limits<double>::infinity(); }
+template <CDecimalConcept CType>
+struct AntiExtrema<CType> {
+ static constexpr CType anti_min() { return CType::GetMaxSentinel(); }
+ static constexpr CType anti_max() { return CType::GetMinSentinel(); }
};
-template <>
-struct AntiExtrema<Decimal32> {
- static constexpr Decimal32 anti_min() { return
BasicDecimal32::GetMaxSentinel(); }
- static constexpr Decimal32 anti_max() { return
BasicDecimal32::GetMinSentinel(); }
-};
-
-template <>
-struct AntiExtrema<Decimal64> {
- static constexpr Decimal64 anti_min() { return
BasicDecimal64::GetMaxSentinel(); }
- static constexpr Decimal64 anti_max() { return
BasicDecimal64::GetMinSentinel(); }
-};
-
-template <>
-struct AntiExtrema<Decimal128> {
- static constexpr Decimal128 anti_min() { return
BasicDecimal128::GetMaxSentinel(); }
- static constexpr Decimal128 anti_max() { return
BasicDecimal128::GetMinSentinel(); }
+template <typename CType>
+struct MinMaxOp {
+ static constexpr CType min(CType a, CType b) { return std::min(a, b); }
+ static constexpr CType max(CType a, CType b) { return std::max(a, b); }
};
-template <>
-struct AntiExtrema<Decimal256> {
- static constexpr Decimal256 anti_min() { return
BasicDecimal256::GetMaxSentinel(); }
- static constexpr Decimal256 anti_max() { return
BasicDecimal256::GetMinSentinel(); }
+template <CFloatingPointConcept CType>
+struct MinMaxOp<CType> {
+ static constexpr CType min(CType a, CType b) { return std::fmin(a, b); }
+ static constexpr CType max(CType a, CType b) { return std::fmax(a, b); }
Review Comment:
No, but we don't either specialize for `Float16` so it compiles. Please see
my other comment.
##########
cpp/src/arrow/type_traits.h:
##########
@@ -1837,4 +1838,23 @@ constexpr bool is_union(const DataType& type) { return
is_union(type.id()); }
/// @}
+/// \addtogroup c-type-concepts
+/// @{
+
+// XXX: To be completed with more concepts as needed.
+
+template <typename T>
+concept CBooleanConcept = std::is_same_v<T, bool>;
Review Comment:
Yes. C++20 `std::same_as` is preferred. Updated.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
