lidavidm commented on a change in pull request #11075:
URL: https://github.com/apache/arrow/pull/11075#discussion_r703512991
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal.cc
##########
@@ -588,20 +614,85 @@ inline std::array<int64_t, 3> GetIsoCalendar(int64_t arg,
Localizer&& localizer)
static_cast<int64_t>(weekday(ymd).iso_encoding())};
}
+template <typename Duration, typename InType>
+struct ISOCalendarWrapper {
+ static Status get(const Scalar& in, std::array<int64_t, 3>& iso_calendar) {
Review comment:
I think this factorization is good, however, just a nit: methods should
use UpperCamelCase unless they're const getters so this should be called Get.
##########
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:
Hmm, it's reasonable. In that case, below, can we do the following, so
it's clear what the bare boolean parameter is for?
```
auto quarter = MakeTemporal<Quarter, TemporalComponentExtract, Int64Type>(
/*enable_date=*/true, "quarter", int64(), &quarter_doc);
```
##########
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:
I will note though, a struct like this can collapse MakeTemporal and
MakeSimpleUnaryTemporal into one function:
```cpp
template <template <typename...> class Op, typename Duration, typename
InType>
struct SimpleUnaryTemporal {
static Status Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out)
{
return SimpleUnary<Op<Duration, InType>>(ctx, batch, out);
}
};
```
You would then replace OutType with `typename... Args` in `MakeTemporal` and
you could replace `MakeSimpleUnaryTemporal<Foo>` with `MakeTemporal<Foo,
SimpleUnaryTemporal>`. Then you wouldn't need two versions of `AddDateKernels`.
Also note that the function creation is only done once on initialization, so
it doesn't really matter whether it's done in the same function or not.
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
##########
@@ -98,24 +136,33 @@ class ScalarTemporalTest : public ::testing::Test {
namespace compute {
-TEST_F(ScalarTemporalTest, TestTemporalComponentExtraction) {
- auto unit = timestamp(TimeUnit::NANO);
- CheckScalarUnary("year", unit, times, int64(), year);
- CheckScalarUnary("month", unit, times, int64(), month);
- CheckScalarUnary("day", unit, times, int64(), day);
- CheckScalarUnary("day_of_week", unit, times, int64(), day_of_week);
- CheckScalarUnary("day_of_year", unit, times, int64(), day_of_year);
- CheckScalarUnary("iso_year", unit, times, int64(), iso_year);
- CheckScalarUnary("iso_week", unit, times, int64(), iso_week);
- CheckScalarUnary("iso_calendar", ArrayFromJSON(unit, times), iso_calendar);
- CheckScalarUnary("quarter", unit, times, int64(), quarter);
- CheckScalarUnary("hour", unit, times, int64(), hour);
- CheckScalarUnary("minute", unit, times, int64(), minute);
- CheckScalarUnary("second", unit, times, int64(), second);
- CheckScalarUnary("millisecond", unit, times, int64(), millisecond);
- CheckScalarUnary("microsecond", unit, times, int64(), microsecond);
- CheckScalarUnary("nanosecond", unit, times, int64(), nanosecond);
- CheckScalarUnary("subsecond", unit, times, float64(), subsecond);
+TEST_F(ScalarTemporalTest, TestTemporalComponentExtractionAllTypes) {
+ std::vector<std::shared_ptr<DataType>> units = {date32(), date64(),
+ timestamp(TimeUnit::NANO)};
+ std::vector<const char*> samples = {date32s, date64s, times};
+ assert(units.size() == samples.size());
Review comment:
Usually we use DCHECK for these sorts of checks (e.g.
`DCHECK_EQ(units.size(), samples.size())`)
--
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]
