bkietz commented on a change in pull request #7461:
URL: https://github.com/apache/arrow/pull/7461#discussion_r441584777
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -807,11 +885,30 @@ ArrayKernelExec SignedInteger(detail::GetTypeId get_id) {
}
}
-// Generate a kernel given a templated functor for base binary types
+// Generate a kernel given a templated functor for base binary types. Generates
+// a single kernel for binary/string and large binary / large string. If your
+// kernel implementation needs access to the specific type at compile time,
+// please use BaseBinarySpecific.
//
// See "Numeric" above for description of the generator functor
template <template <typename...> class Generator, typename Type0, typename...
Args>
ArrayKernelExec BaseBinary(detail::GetTypeId get_id) {
Review comment:
```suggestion
ArrayKernelExec GenerateForBaseBinary(detail::GetTypeId get_id) {
```
##########
File path: cpp/src/arrow/type.h
##########
@@ -1251,6 +1256,7 @@ class ARROW_EXPORT TimestampType : public TemporalType,
public ParametricType {
static constexpr Type::type type_id = Type::TIMESTAMP;
using c_type = int64_t;
+ using StorageType = Int64Type;
Review comment:
```suggestion
using PhysicalType = Int64Type;
```
?
##########
File path: cpp/src/arrow/compute/kernels/scalar_compare.cc
##########
@@ -54,72 +56,106 @@ struct GreaterEqual {
}
};
-struct Less {
- template <typename T>
- static constexpr bool Call(KernelContext*, const T& left, const T& right) {
- return left < right;
- }
-};
-
-struct LessEqual {
- template <typename T>
- static constexpr bool Call(KernelContext*, const T& left, const T& right) {
- return left <= right;
- }
-};
+// Implement Greater, GreaterEqual by flipping arguments to Less, LessEqual
Review comment:
```suggestion
// Implement Less, LessEqual by flipping arguments to Greater, GreaterEqual
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_compare.cc
##########
@@ -54,72 +56,106 @@ struct GreaterEqual {
}
};
-struct Less {
- template <typename T>
- static constexpr bool Call(KernelContext*, const T& left, const T& right) {
- return left < right;
- }
-};
-
-struct LessEqual {
- template <typename T>
- static constexpr bool Call(KernelContext*, const T& left, const T& right) {
- return left <= right;
- }
-};
+// Implement Greater, GreaterEqual by flipping arguments to Less, LessEqual
-template <typename InType, typename Op>
-void AddCompare(const std::shared_ptr<DataType>& ty, ScalarFunction* func) {
- ArrayKernelExec exec = codegen::ScalarBinaryEqualTypes<BooleanType, InType,
Op>::Exec;
- DCHECK_OK(func->AddKernel({ty, ty}, boolean(), exec));
+template <typename Op>
Review comment:
It would improve readability if we formalized the `Op` concept and named
them consistently. Even within this PR these are also called `Generator`s. It's
not consistently made clear what functions they're expected to provide.
https://issues.apache.org/jira/browse/ARROW-9161
##########
File path: cpp/src/arrow/scalar.h
##########
@@ -237,19 +246,17 @@ struct ARROW_EXPORT FixedSizeBinaryScalar : public
BinaryScalar {
explicit FixedSizeBinaryScalar(std::shared_ptr<DataType> type) :
BinaryScalar(type) {}
};
-template <typename T>
-struct ARROW_EXPORT TemporalScalar : public Scalar {
- using Scalar::Scalar;
+template <typename T, typename StorageType = typename T::StorageType,
+ typename Enable = void>
+struct ARROW_EXPORT TemporalScalar : internal::PrimitiveScalar<StorageType> {
Review comment:
I'm not sure this use of inheritance is kosher. This implies
`TimestampScalar isa Int64Scalar`, which is a relationship not present in
parallel hierarchies (Array, DataType, Builder) and is therefore likely to be
confusing. Why not use `TemporalScalar : PrimitiveScalar<T>`?
##########
File path: cpp/src/arrow/compute/kernels/scalar_compare.cc
##########
@@ -54,72 +56,106 @@ struct GreaterEqual {
}
};
-struct Less {
- template <typename T>
- static constexpr bool Call(KernelContext*, const T& left, const T& right) {
- return left < right;
- }
-};
-
-struct LessEqual {
- template <typename T>
- static constexpr bool Call(KernelContext*, const T& left, const T& right) {
- return left <= right;
- }
-};
+// Implement Greater, GreaterEqual by flipping arguments to Less, LessEqual
-template <typename InType, typename Op>
-void AddCompare(const std::shared_ptr<DataType>& ty, ScalarFunction* func) {
- ArrayKernelExec exec = codegen::ScalarBinaryEqualTypes<BooleanType, InType,
Op>::Exec;
- DCHECK_OK(func->AddKernel({ty, ty}, boolean(), exec));
+template <typename Op>
+void AddIntegerCompare(const std::shared_ptr<DataType>& ty, ScalarFunction*
func) {
+ auto exec =
+ codegen::IntegerBased<codegen::ScalarBinaryEqualTypes, BooleanType,
Op>(*ty);
+ DCHECK_OK(func->AddKernel({ty, ty}, boolean(), std::move(exec)));
}
-template <typename Op>
-void AddTimestampComparisons(ScalarFunction* func) {
- ArrayKernelExec exec =
- codegen::ScalarBinaryEqualTypes<BooleanType, TimestampType, Op>::Exec;
- for (auto unit : {TimeUnit::SECOND, TimeUnit::MILLI, TimeUnit::MICRO,
TimeUnit::NANO}) {
- InputType in_type(match::TimestampUnit(unit));
- DCHECK_OK(func->AddKernel({in_type, in_type}, boolean(), exec));
- }
+template <typename InType, typename Op>
+void AddGenericCompare(const std::shared_ptr<DataType>& ty, ScalarFunction*
func) {
+ DCHECK_OK(
+ func->AddKernel({ty, ty}, boolean(),
+ codegen::ScalarBinaryEqualTypes<BooleanType, InType,
Op>::Exec));
}
template <typename Op>
-void MakeCompareFunction(std::string name, FunctionRegistry* registry) {
+std::shared_ptr<ScalarFunction> MakeCompareFunction(std::string name) {
auto func = std::make_shared<ScalarFunction>(name, Arity::Binary());
DCHECK_OK(func->AddKernel(
{boolean(), boolean()}, boolean(),
codegen::ScalarBinary<BooleanType, BooleanType, BooleanType, Op>::Exec));
- for (const std::shared_ptr<DataType>& ty : NumericTypes()) {
- auto exec = codegen::Numeric<codegen::ScalarBinaryEqualTypes, BooleanType,
Op>(*ty);
- DCHECK_OK(func->AddKernel({ty, ty}, boolean(), exec));
+ for (const std::shared_ptr<DataType>& ty : IntTypes()) {
+ AddIntegerCompare<Op>(ty, func.get());
+ }
+ AddIntegerCompare<Op>(date32(), func.get());
+ AddIntegerCompare<Op>(date64(), func.get());
+
+ AddGenericCompare<FloatType, Op>(float32(), func.get());
+ AddGenericCompare<DoubleType, Op>(float64(), func.get());
+
+ // Add timestamp kernels
+ for (auto unit : {TimeUnit::SECOND, TimeUnit::MILLI, TimeUnit::MICRO,
TimeUnit::NANO}) {
+ InputType in_type(match::TimestampTypeUnit(unit));
+ auto exec =
+ codegen::IntegerBased<codegen::ScalarBinaryEqualTypes, BooleanType,
Op>(int64());
+ DCHECK_OK(func->AddKernel({in_type, in_type}, boolean(), std::move(exec)));
+ }
+
+ // Duration
+ for (auto unit : {TimeUnit::SECOND, TimeUnit::MILLI, TimeUnit::MICRO,
TimeUnit::NANO}) {
+ InputType in_type(match::DurationTypeUnit(unit));
+ auto exec =
+ codegen::IntegerBased<codegen::ScalarBinaryEqualTypes, BooleanType,
Op>(int64());
+ DCHECK_OK(func->AddKernel({in_type, in_type}, boolean(), std::move(exec)));
+ }
+
+ // Time32 and Time64
+ for (auto unit : {TimeUnit::SECOND, TimeUnit::MILLI}) {
+ InputType in_type(match::Time32TypeUnit(unit));
+ auto exec =
+ codegen::IntegerBased<codegen::ScalarBinaryEqualTypes, BooleanType,
Op>(int32());
+ DCHECK_OK(func->AddKernel({in_type, in_type}, boolean(), std::move(exec)));
+ }
+ for (auto unit : {TimeUnit::MICRO, TimeUnit::NANO}) {
+ InputType in_type(match::Time64TypeUnit(unit));
+ auto exec =
+ codegen::IntegerBased<codegen::ScalarBinaryEqualTypes, BooleanType,
Op>(int64());
+ DCHECK_OK(func->AddKernel({in_type, in_type}, boolean(), std::move(exec)));
}
+
for (const std::shared_ptr<DataType>& ty : BaseBinaryTypes()) {
auto exec =
codegen::BaseBinary<codegen::ScalarBinaryEqualTypes, BooleanType,
Op>(*ty);
- DCHECK_OK(func->AddKernel({ty, ty}, boolean(), exec));
+ DCHECK_OK(func->AddKernel({ty, ty}, boolean(), std::move(exec)));
}
- // Temporal types requires some care because cross-unit comparisons with
- // everything but DATE32 and DATE64 are not implemented yet
- AddCompare<Date32Type, Op>(date32(), func.get());
- AddCompare<Date64Type, Op>(date64(), func.get());
- AddTimestampComparisons<Op>(func.get());
-
// TODO: Leave time32, time64, and duration for follow up work
Review comment:
```suggestion
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
