lidavidm commented on a change in pull request #11075:
URL: https://github.com/apache/arrow/pull/11075#discussion_r701897054
##########
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:
Would it be possible to avoid specialization by instead making the
helper functions used work with timestamps and dates? For instance, you could
modify GetInputTimezone to be `GetInputTimezone<InType>` where it would always
return an empty string for `InType == Date32Type`.
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
##########
@@ -98,24 +136,32 @@ 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, TestTemporalComponentExtractionPERCY) {
Review comment:
Don't forget to change this back :)
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal.cc
##########
@@ -670,26 +833,36 @@ std::shared_ptr<ScalarFunction> MakeTemporal(
auto func =
std::make_shared<ScalarFunction>(name, Arity::Unary(), doc,
default_options);
+ {
Review comment:
Frankly I think kernels like `hour` should just not work on dates - it's
not meaningful.
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal.cc
##########
@@ -466,8 +492,90 @@ struct Nanosecond {
// Convert timestamps to a string representation with an arbitrary format
#ifndef _WIN32
-template <typename Duration>
+template <typename Duration, typename InType>
struct Strftime {
Review comment:
nit: since this is for dates only, maybe have the base `struct Strftime`
have no implementation, and implement both dates and timestamps as
specializations?
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal.cc
##########
@@ -466,8 +492,90 @@ struct Nanosecond {
// Convert timestamps to a string representation with an arbitrary format
#ifndef _WIN32
-template <typename Duration>
+template <typename Duration, typename InType>
struct Strftime {
Review comment:
Actually, given that the regular strftime kernel won't format timestamps
without timezones, I wonder if it's meaningful to have strftime work on dates.
I would say if you want a string representation of a date, you should cast,
since strftime would let you do misleading things like trying to extract the
hour from a date.
##########
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:
(I suppose you'd also need to change functions to templates to be able
to specialize things like that...)
--
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]
