lidavidm commented on a change in pull request #11075:
URL: https://github.com/apache/arrow/pull/11075#discussion_r702057037
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal.cc
##########
@@ -588,8 +701,58 @@ inline std::array<int64_t, 3> GetIsoCalendar(int64_t arg,
Localizer&& localizer)
static_cast<int64_t>(weekday(ymd).iso_encoding())};
}
-template <typename Duration>
+template <typename Duration, typename InType>
struct ISOCalendar {
Review comment:
We could get around that with a static string somewhere, or by returning
`const std::string*`. Or the actual implementation could be factored out, and
the date kernel could call it with a literal string. At the very least, I'm not
super enthused about essentially copy-pasting this kernel.
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal.cc
##########
@@ -660,36 +736,46 @@ struct ISOCalendar {
template <
template <typename...> class Op,
- template <template <typename...> class OpExec, typename Duration, typename
OutType>
+ template <template <typename...> class OpExec, typename Duration, typename
InType, typename OutType>
class ExecTemplate,
typename OutType>
-std::shared_ptr<ScalarFunction> MakeTemporal(
+std::shared_ptr<ScalarFunction> MakeTemporal(bool enable_date,
Review comment:
Instead of a bool, I think it'd be cleaner to split the date kernel
registration into a separate function:
```cpp
template <
template <typename...> class Op,
template <template <typename...> class OpExec, typename Duration,
typename InType, typename OutType>
class ExecTemplate,
typename OutType>
std::shared_ptr<ScalarFunction> AddDateKernels(ScalarFunction* func, const
std::shared_ptr<arrow::DataType>& out_type, KernelInit init = nullptr) {
{
auto exec = ExecTemplate<Op, days, Date32Type, OutType>::Exec;
DCHECK_OK(func->AddKernel({date32()}, out_Type, std::move(exec), init);
}
{
auto exec = ExecTemplate<Op, std::chrono::milliseconds, Date64Type,
OutType>::Exec;
DCHECK_OK(func->AddKernel({date64()}, out_Type, std::move(exec), init);
}
}
```
Then you can just call (for instance) `AddDateKernels<Year,
TemporalComponentExtract, Int64Type>(year.get(), int64());` below.
--
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]
