ARROW-13138: [C++][R] Implement extract temporal components (year, month, day, etc) from date32/64 types

[aucahuasi]

https://issues.apache.org/jira/browse/ARROW-13138

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13138

[lidavidm]

Thanks for tackling this, this looks like a great start. I think my main questions are 1) whether we should support strftime, hour, minute, etc. on dates as I don't think they're meaningful, and 2) whether we can share the implementation of ISOCalendar instead of duplicating it.

[lidavidm]

nit: since this is for dates only, maybe have the base struct Strftime have no implementation, and implement both dates and timestamps as specializations?

[lidavidm]

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.

[aucahuasi]

I think you are right. Do you know if we have tests for casting date32/64 to strings?

[lidavidm]

We do, they're a bit thin though:

arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc

Line 1245 in 5ead375

TEST(Cast, DateToString) {

[rok]

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.

There's work to have Strftime work on timestamps without timezones (assuming UTC)
#10998. It's a philosophical discussion if it's correct or not but it will be available.

[lidavidm]

Sorry for the back-and-forth @aucahuasi. However, it might be prudent to split strftime support into a separate JIRA to minimize conflicts with the existing PR and so we can evaluate how best to share the implementation (to avoid too much code duplication).

[aucahuasi]

No worries @lidavidm :)
Indeed, it makes sense to have a new jira. I can create the ticket if there is none.

[lidavidm]

I don't think there's a JIRA so please do file one.

[aucahuasi]

Here is the jira ticket for strftime, thanks @lidavidm@rok
https://issues.apache.org/jira/browse/ARROW-13916

[rok]

The Strftime ticket was merged yesterday so maybe something has changed here. Dunno.

[lidavidm]

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.

[lidavidm]

(I suppose you'd also need to change functions to templates to be able to specialize things like that...)

[aucahuasi]

Thanks @lidavidm !

I think the provided implementation (using specialization) is good enough.
Please note that this function is returning a const ref string:
const std::string& GetInputTimezone
So the compiler will throw an error if we want to return local empty strings.
And if I change the declaration to return a string (without const ref) we will be copying the timezones instead of using the refs.

Also, for the case of ISOCalendar with TimestampType we had this code:
string timezone = GetInputTimezone
and we had been performing a copy.
So I changed that to
const auto& timezone = GetInputTimezone

[lidavidm]

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.

[aucahuasi]

Sure, that's a good idea, I can try to have a single implementation.
Where should I put the empty static string? in which file?

[lidavidm]

It would go in this file, though FWIW, it would probably be easiest to just factor things into functions since these are all static methods anyways.

[aucahuasi]

@lidavidm I think this last version reuse the kernel logic and it specializes only the parts related to the timestamp logic. Please let me know if it is good enough.
TODO: I need to fix the some R tests (I'll tackle this tomorrow)

[lidavidm]

Frankly I think kernels like hour should just not work on dates - it's not meaningful.

[aucahuasi]

Make sense, pandas doesn't has that behavior too

pd.to_datetime("2019-01-03", format='%Y-%m-%d', errors='coerce').date().hour()
AttributeError: 'datetime.date'objecthasnoattribute'hour'

I'll remove the support of date32/64 for hour, min, sec ...

[lidavidm]

Instead of a bool, I think it'd be cleaner to split the date kernel registration into a separate function:

template <
template <typename...> classOp,
template <template <typename...> classOpExec, typename Duration, typename InType, typename OutType>
classExecTemplate,
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.

[aucahuasi]

Thanks, but I suggest to keep the provided implementation in this PR:

• It requires less code/It change less parts.
• It integrates well into the MakeTemporal* functions.
• It is setting up the date kernels (optionally) at the moment of creating the function.

Also, I think that if I add AddDateKernels we would need 2 versions: one for MakeTemporal and other for MakeSimpleUnaryTemporal.
So I think it makes sense to keep the current code. Let me know what do you think here @lidavidm

[lidavidm]

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);

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[aucahuasi]

Thanks, I added /*enable_date=*/ to indicate the intention of the boolean parameter.
Regarding the SimpleUnaryTemporal struct, sounds like a nice idea for a future refactor, however if you feel strongly about carrying out these sort of changes in this PR, let me know please!

[lidavidm]

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.

[lidavidm]

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);

[lidavidm]

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.

[aucahuasi]

Done, thanks @lidavidm@rok !

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[lidavidm]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case. Seems this can be changed to expect_dplyr_equal:

arrow/r/tests/testthat/test-dplyr-lubridate.R

Lines 37 to 48 in 2ef0edc

	# We can support this feature when ARROW-13138 is resolved
	test_that("date32 objects are not supported", {
	date<- ymd("2017-01-01")
	df<-tibble::tibble(date=date)
	expect_error(
	Table$create(df) %>%
	mutate(x= year(date)) %>%
	collect(),
	"Function year has no kernel matching input types"
	)
	})

[lidavidm]

Thanks!

One last thing that I forgot earlier, sorry: we should update compute.rst as well: https://github.com/apache/arrow/blob/f40856a768f2c397082da70af034080994587807/docs/source/cpp/compute.rst#temporal-component-extraction

[lidavidm]

Oh, it also seems we need to rebase here.

[aucahuasi]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case.
Seems this can be changed to expect_dplyr_equal:

@lidavidm Thank you! I fixed the issue with the R test and added some additional R tests!

[aucahuasi]

One last thing that I forgot earlier, sorry: we should update compute.rst as well:

Done!

[aucahuasi]

It seems there are some build issues on windows. Tomorrow I'll check that!

[lidavidm]

Note the TestNonexistentTimezone test is also failing on Linux.

[aucahuasi]

Note the TestNonexistentTimezone test is also failing on Linux.

Thanks, fixed!

[rok]

Looks good. Appveyor fail doesn't seem related.

It would probably be good to also add Python tests.

[lidavidm]

LGTM, thank you.

I filed https://issues.apache.org/jira/browse/ARROW-13957 for the flaky AppVeyor test.

I'm not sure if we need explicit Python tests, maybe comparing against Pandas would be nice, but unlike R we don't have higher level bindings we're trying to test.

[pitrou]

For later: either pass a const reference or a non-const pointer. Non-const references require more care from the caller.

[pitrou]

Same here: visit_value should be passed by pointer.

[lidavidm]

Thanks @pitrou, I filed ARROW-13960

[aucahuasi]

Thanks @lidavidm@rok@pitrou !

[aucahuasi]

https://issues.apache.org/jira/browse/ARROW-13138

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13138

[lidavidm]

Thanks for tackling this, this looks like a great start. I think my main questions are 1) whether we should support strftime, hour, minute, etc. on dates as I don't think they're meaningful, and 2) whether we can share the implementation of ISOCalendar instead of duplicating it.

[lidavidm]

nit: since this is for dates only, maybe have the base struct Strftime have no implementation, and implement both dates and timestamps as specializations?

[lidavidm]

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.

[aucahuasi]

I think you are right. Do you know if we have tests for casting date32/64 to strings?

[lidavidm]

We do, they're a bit thin though:

arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc

Line 1245 in 5ead375

TEST(Cast, DateToString) {

[rok]

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.

There's work to have Strftime work on timestamps without timezones (assuming UTC)
#10998. It's a philosophical discussion if it's correct or not but it will be available.

[lidavidm]

Sorry for the back-and-forth @aucahuasi. However, it might be prudent to split strftime support into a separate JIRA to minimize conflicts with the existing PR and so we can evaluate how best to share the implementation (to avoid too much code duplication).

[aucahuasi]

No worries @lidavidm :)
Indeed, it makes sense to have a new jira. I can create the ticket if there is none.

[lidavidm]

I don't think there's a JIRA so please do file one.

[aucahuasi]

Here is the jira ticket for strftime, thanks @lidavidm@rok
https://issues.apache.org/jira/browse/ARROW-13916

[rok]

The Strftime ticket was merged yesterday so maybe something has changed here. Dunno.

[lidavidm]

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.

[lidavidm]

(I suppose you'd also need to change functions to templates to be able to specialize things like that...)

[aucahuasi]

Thanks @lidavidm !

I think the provided implementation (using specialization) is good enough.
Please note that this function is returning a const ref string:
const std::string& GetInputTimezone
So the compiler will throw an error if we want to return local empty strings.
And if I change the declaration to return a string (without const ref) we will be copying the timezones instead of using the refs.

Also, for the case of ISOCalendar with TimestampType we had this code:
string timezone = GetInputTimezone
and we had been performing a copy.
So I changed that to
const auto& timezone = GetInputTimezone

[lidavidm]

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.

[aucahuasi]

Sure, that's a good idea, I can try to have a single implementation.
Where should I put the empty static string? in which file?

[lidavidm]

It would go in this file, though FWIW, it would probably be easiest to just factor things into functions since these are all static methods anyways.

[aucahuasi]

@lidavidm I think this last version reuse the kernel logic and it specializes only the parts related to the timestamp logic. Please let me know if it is good enough.
TODO: I need to fix the some R tests (I'll tackle this tomorrow)

[lidavidm]

Frankly I think kernels like hour should just not work on dates - it's not meaningful.

[aucahuasi]

Make sense, pandas doesn't has that behavior too

pd.to_datetime("2019-01-03", format='%Y-%m-%d', errors='coerce').date().hour()
AttributeError: 'datetime.date'objecthasnoattribute'hour'

I'll remove the support of date32/64 for hour, min, sec ...

[lidavidm]

Instead of a bool, I think it'd be cleaner to split the date kernel registration into a separate function:

template <
template <typename...> classOp,
template <template <typename...> classOpExec, typename Duration, typename InType, typename OutType>
classExecTemplate,
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.

[aucahuasi]

Thanks, but I suggest to keep the provided implementation in this PR:

• It requires less code/It change less parts.
• It integrates well into the MakeTemporal* functions.
• It is setting up the date kernels (optionally) at the moment of creating the function.

Also, I think that if I add AddDateKernels we would need 2 versions: one for MakeTemporal and other for MakeSimpleUnaryTemporal.
So I think it makes sense to keep the current code. Let me know what do you think here @lidavidm

[lidavidm]

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);

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[aucahuasi]

Thanks, I added /*enable_date=*/ to indicate the intention of the boolean parameter.
Regarding the SimpleUnaryTemporal struct, sounds like a nice idea for a future refactor, however if you feel strongly about carrying out these sort of changes in this PR, let me know please!

[lidavidm]

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.

[lidavidm]

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);

[lidavidm]

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.

[aucahuasi]

Done, thanks @lidavidm@rok !

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[lidavidm]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case. Seems this can be changed to expect_dplyr_equal:

arrow/r/tests/testthat/test-dplyr-lubridate.R

Lines 37 to 48 in 2ef0edc

	# We can support this feature when ARROW-13138 is resolved
	test_that("date32 objects are not supported", {
	date<- ymd("2017-01-01")
	df<-tibble::tibble(date=date)
	expect_error(
	Table$create(df) %>%
	mutate(x= year(date)) %>%
	collect(),
	"Function year has no kernel matching input types"
	)
	})

[lidavidm]

Thanks!

One last thing that I forgot earlier, sorry: we should update compute.rst as well: https://github.com/apache/arrow/blob/f40856a768f2c397082da70af034080994587807/docs/source/cpp/compute.rst#temporal-component-extraction

[lidavidm]

Oh, it also seems we need to rebase here.

[aucahuasi]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case.
Seems this can be changed to expect_dplyr_equal:

@lidavidm Thank you! I fixed the issue with the R test and added some additional R tests!

[aucahuasi]

One last thing that I forgot earlier, sorry: we should update compute.rst as well:

Done!

[aucahuasi]

It seems there are some build issues on windows. Tomorrow I'll check that!

[lidavidm]

Note the TestNonexistentTimezone test is also failing on Linux.

[aucahuasi]

Note the TestNonexistentTimezone test is also failing on Linux.

Thanks, fixed!

[rok]

Looks good. Appveyor fail doesn't seem related.

It would probably be good to also add Python tests.

[lidavidm]

LGTM, thank you.

I filed https://issues.apache.org/jira/browse/ARROW-13957 for the flaky AppVeyor test.

I'm not sure if we need explicit Python tests, maybe comparing against Pandas would be nice, but unlike R we don't have higher level bindings we're trying to test.

[pitrou]

For later: either pass a const reference or a non-const pointer. Non-const references require more care from the caller.

[pitrou]

Same here: visit_value should be passed by pointer.

[lidavidm]

Thanks @pitrou, I filed ARROW-13960

[aucahuasi]

Thanks @lidavidm@rok@pitrou !

[aucahuasi]

https://issues.apache.org/jira/browse/ARROW-13138

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13138

[lidavidm]

Thanks for tackling this, this looks like a great start. I think my main questions are 1) whether we should support strftime, hour, minute, etc. on dates as I don't think they're meaningful, and 2) whether we can share the implementation of ISOCalendar instead of duplicating it.

[lidavidm]

nit: since this is for dates only, maybe have the base struct Strftime have no implementation, and implement both dates and timestamps as specializations?

[lidavidm]

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.

[aucahuasi]

I think you are right. Do you know if we have tests for casting date32/64 to strings?

[lidavidm]

We do, they're a bit thin though:

arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc

Line 1245 in 5ead375

TEST(Cast, DateToString) {

[rok]

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.

There's work to have Strftime work on timestamps without timezones (assuming UTC)
#10998. It's a philosophical discussion if it's correct or not but it will be available.

[lidavidm]

Sorry for the back-and-forth @aucahuasi. However, it might be prudent to split strftime support into a separate JIRA to minimize conflicts with the existing PR and so we can evaluate how best to share the implementation (to avoid too much code duplication).

[aucahuasi]

No worries @lidavidm :)
Indeed, it makes sense to have a new jira. I can create the ticket if there is none.

[lidavidm]

I don't think there's a JIRA so please do file one.

[aucahuasi]

Here is the jira ticket for strftime, thanks @lidavidm@rok
https://issues.apache.org/jira/browse/ARROW-13916

[rok]

The Strftime ticket was merged yesterday so maybe something has changed here. Dunno.

[lidavidm]

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.

[lidavidm]

(I suppose you'd also need to change functions to templates to be able to specialize things like that...)

[aucahuasi]

Thanks @lidavidm !

I think the provided implementation (using specialization) is good enough.
Please note that this function is returning a const ref string:
const std::string& GetInputTimezone
So the compiler will throw an error if we want to return local empty strings.
And if I change the declaration to return a string (without const ref) we will be copying the timezones instead of using the refs.

Also, for the case of ISOCalendar with TimestampType we had this code:
string timezone = GetInputTimezone
and we had been performing a copy.
So I changed that to
const auto& timezone = GetInputTimezone

[lidavidm]

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.

[aucahuasi]

Sure, that's a good idea, I can try to have a single implementation.
Where should I put the empty static string? in which file?

[lidavidm]

It would go in this file, though FWIW, it would probably be easiest to just factor things into functions since these are all static methods anyways.

[aucahuasi]

@lidavidm I think this last version reuse the kernel logic and it specializes only the parts related to the timestamp logic. Please let me know if it is good enough.
TODO: I need to fix the some R tests (I'll tackle this tomorrow)

[lidavidm]

Frankly I think kernels like hour should just not work on dates - it's not meaningful.

[aucahuasi]

Make sense, pandas doesn't has that behavior too

pd.to_datetime("2019-01-03", format='%Y-%m-%d', errors='coerce').date().hour()
AttributeError: 'datetime.date'objecthasnoattribute'hour'

I'll remove the support of date32/64 for hour, min, sec ...

[lidavidm]

Instead of a bool, I think it'd be cleaner to split the date kernel registration into a separate function:

template <
template <typename...> classOp,
template <template <typename...> classOpExec, typename Duration, typename InType, typename OutType>
classExecTemplate,
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.

[aucahuasi]

Thanks, but I suggest to keep the provided implementation in this PR:

• It requires less code/It change less parts.
• It integrates well into the MakeTemporal* functions.
• It is setting up the date kernels (optionally) at the moment of creating the function.

Also, I think that if I add AddDateKernels we would need 2 versions: one for MakeTemporal and other for MakeSimpleUnaryTemporal.
So I think it makes sense to keep the current code. Let me know what do you think here @lidavidm

[lidavidm]

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);

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[aucahuasi]

Thanks, I added /*enable_date=*/ to indicate the intention of the boolean parameter.
Regarding the SimpleUnaryTemporal struct, sounds like a nice idea for a future refactor, however if you feel strongly about carrying out these sort of changes in this PR, let me know please!

[lidavidm]

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.

[lidavidm]

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);

[lidavidm]

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.

[aucahuasi]

Done, thanks @lidavidm@rok !

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[lidavidm]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case. Seems this can be changed to expect_dplyr_equal:

arrow/r/tests/testthat/test-dplyr-lubridate.R

Lines 37 to 48 in 2ef0edc

	# We can support this feature when ARROW-13138 is resolved
	test_that("date32 objects are not supported", {
	date<- ymd("2017-01-01")
	df<-tibble::tibble(date=date)
	expect_error(
	Table$create(df) %>%
	mutate(x= year(date)) %>%
	collect(),
	"Function year has no kernel matching input types"
	)
	})

[lidavidm]

Thanks!

One last thing that I forgot earlier, sorry: we should update compute.rst as well: https://github.com/apache/arrow/blob/f40856a768f2c397082da70af034080994587807/docs/source/cpp/compute.rst#temporal-component-extraction

[lidavidm]

Oh, it also seems we need to rebase here.

[aucahuasi]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case.
Seems this can be changed to expect_dplyr_equal:

@lidavidm Thank you! I fixed the issue with the R test and added some additional R tests!

[aucahuasi]

One last thing that I forgot earlier, sorry: we should update compute.rst as well:

Done!

[aucahuasi]

It seems there are some build issues on windows. Tomorrow I'll check that!

[lidavidm]

Note the TestNonexistentTimezone test is also failing on Linux.

[aucahuasi]

Note the TestNonexistentTimezone test is also failing on Linux.

Thanks, fixed!

[rok]

Looks good. Appveyor fail doesn't seem related.

It would probably be good to also add Python tests.

[lidavidm]

LGTM, thank you.

I filed https://issues.apache.org/jira/browse/ARROW-13957 for the flaky AppVeyor test.

I'm not sure if we need explicit Python tests, maybe comparing against Pandas would be nice, but unlike R we don't have higher level bindings we're trying to test.

[pitrou]

For later: either pass a const reference or a non-const pointer. Non-const references require more care from the caller.

[pitrou]

Same here: visit_value should be passed by pointer.

[lidavidm]

Thanks @pitrou, I filed ARROW-13960

[aucahuasi]

Thanks @lidavidm@rok@pitrou !

[aucahuasi]

https://issues.apache.org/jira/browse/ARROW-13138

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13138

[lidavidm]

Thanks for tackling this, this looks like a great start. I think my main questions are 1) whether we should support strftime, hour, minute, etc. on dates as I don't think they're meaningful, and 2) whether we can share the implementation of ISOCalendar instead of duplicating it.

[lidavidm]

nit: since this is for dates only, maybe have the base struct Strftime have no implementation, and implement both dates and timestamps as specializations?

[lidavidm]

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.

[aucahuasi]

I think you are right. Do you know if we have tests for casting date32/64 to strings?

[lidavidm]

We do, they're a bit thin though:

arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc

Line 1245 in 5ead375

TEST(Cast, DateToString) {

[rok]

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.

There's work to have Strftime work on timestamps without timezones (assuming UTC)
#10998. It's a philosophical discussion if it's correct or not but it will be available.

[lidavidm]

Sorry for the back-and-forth @aucahuasi. However, it might be prudent to split strftime support into a separate JIRA to minimize conflicts with the existing PR and so we can evaluate how best to share the implementation (to avoid too much code duplication).

[aucahuasi]

No worries @lidavidm :)
Indeed, it makes sense to have a new jira. I can create the ticket if there is none.

[lidavidm]

I don't think there's a JIRA so please do file one.

[aucahuasi]

Here is the jira ticket for strftime, thanks @lidavidm@rok
https://issues.apache.org/jira/browse/ARROW-13916

[rok]

The Strftime ticket was merged yesterday so maybe something has changed here. Dunno.

[lidavidm]

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.

[lidavidm]

(I suppose you'd also need to change functions to templates to be able to specialize things like that...)

[aucahuasi]

Thanks @lidavidm !

I think the provided implementation (using specialization) is good enough.
Please note that this function is returning a const ref string:
const std::string& GetInputTimezone
So the compiler will throw an error if we want to return local empty strings.
And if I change the declaration to return a string (without const ref) we will be copying the timezones instead of using the refs.

Also, for the case of ISOCalendar with TimestampType we had this code:
string timezone = GetInputTimezone
and we had been performing a copy.
So I changed that to
const auto& timezone = GetInputTimezone

[lidavidm]

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.

[aucahuasi]

Sure, that's a good idea, I can try to have a single implementation.
Where should I put the empty static string? in which file?

[lidavidm]

It would go in this file, though FWIW, it would probably be easiest to just factor things into functions since these are all static methods anyways.

[aucahuasi]

@lidavidm I think this last version reuse the kernel logic and it specializes only the parts related to the timestamp logic. Please let me know if it is good enough.
TODO: I need to fix the some R tests (I'll tackle this tomorrow)

[lidavidm]

Frankly I think kernels like hour should just not work on dates - it's not meaningful.

[aucahuasi]

Make sense, pandas doesn't has that behavior too

pd.to_datetime("2019-01-03", format='%Y-%m-%d', errors='coerce').date().hour()
AttributeError: 'datetime.date'objecthasnoattribute'hour'

I'll remove the support of date32/64 for hour, min, sec ...

[lidavidm]

Instead of a bool, I think it'd be cleaner to split the date kernel registration into a separate function:

template <
template <typename...> classOp,
template <template <typename...> classOpExec, typename Duration, typename InType, typename OutType>
classExecTemplate,
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.

[aucahuasi]

Thanks, but I suggest to keep the provided implementation in this PR:

• It requires less code/It change less parts.
• It integrates well into the MakeTemporal* functions.
• It is setting up the date kernels (optionally) at the moment of creating the function.

Also, I think that if I add AddDateKernels we would need 2 versions: one for MakeTemporal and other for MakeSimpleUnaryTemporal.
So I think it makes sense to keep the current code. Let me know what do you think here @lidavidm

[lidavidm]

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);

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[aucahuasi]

Thanks, I added /*enable_date=*/ to indicate the intention of the boolean parameter.
Regarding the SimpleUnaryTemporal struct, sounds like a nice idea for a future refactor, however if you feel strongly about carrying out these sort of changes in this PR, let me know please!

[lidavidm]

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.

[lidavidm]

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);

[lidavidm]

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.

[aucahuasi]

Done, thanks @lidavidm@rok !

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[lidavidm]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case. Seems this can be changed to expect_dplyr_equal:

arrow/r/tests/testthat/test-dplyr-lubridate.R

Lines 37 to 48 in 2ef0edc

	# We can support this feature when ARROW-13138 is resolved
	test_that("date32 objects are not supported", {
	date<- ymd("2017-01-01")
	df<-tibble::tibble(date=date)
	expect_error(
	Table$create(df) %>%
	mutate(x= year(date)) %>%
	collect(),
	"Function year has no kernel matching input types"
	)
	})

[lidavidm]

Thanks!

One last thing that I forgot earlier, sorry: we should update compute.rst as well: https://github.com/apache/arrow/blob/f40856a768f2c397082da70af034080994587807/docs/source/cpp/compute.rst#temporal-component-extraction

[lidavidm]

Oh, it also seems we need to rebase here.

[aucahuasi]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case.
Seems this can be changed to expect_dplyr_equal:

@lidavidm Thank you! I fixed the issue with the R test and added some additional R tests!

[aucahuasi]

One last thing that I forgot earlier, sorry: we should update compute.rst as well:

Done!

[aucahuasi]

It seems there are some build issues on windows. Tomorrow I'll check that!

[lidavidm]

Note the TestNonexistentTimezone test is also failing on Linux.

[aucahuasi]

Note the TestNonexistentTimezone test is also failing on Linux.

Thanks, fixed!

[rok]

Looks good. Appveyor fail doesn't seem related.

It would probably be good to also add Python tests.

[lidavidm]

LGTM, thank you.

I filed https://issues.apache.org/jira/browse/ARROW-13957 for the flaky AppVeyor test.

I'm not sure if we need explicit Python tests, maybe comparing against Pandas would be nice, but unlike R we don't have higher level bindings we're trying to test.

[pitrou]

For later: either pass a const reference or a non-const pointer. Non-const references require more care from the caller.

[pitrou]

Same here: visit_value should be passed by pointer.

[lidavidm]

Thanks @pitrou, I filed ARROW-13960

[aucahuasi]

Thanks @lidavidm@rok@pitrou !

[aucahuasi]

https://issues.apache.org/jira/browse/ARROW-13138

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13138

[lidavidm]

Thanks for tackling this, this looks like a great start. I think my main questions are 1) whether we should support strftime, hour, minute, etc. on dates as I don't think they're meaningful, and 2) whether we can share the implementation of ISOCalendar instead of duplicating it.

[lidavidm]

nit: since this is for dates only, maybe have the base struct Strftime have no implementation, and implement both dates and timestamps as specializations?

[lidavidm]

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.

[aucahuasi]

I think you are right. Do you know if we have tests for casting date32/64 to strings?

[lidavidm]

We do, they're a bit thin though:

arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc

Line 1245 in 5ead375

TEST(Cast, DateToString) {

[rok]

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.

There's work to have Strftime work on timestamps without timezones (assuming UTC)
#10998. It's a philosophical discussion if it's correct or not but it will be available.

[lidavidm]

Sorry for the back-and-forth @aucahuasi. However, it might be prudent to split strftime support into a separate JIRA to minimize conflicts with the existing PR and so we can evaluate how best to share the implementation (to avoid too much code duplication).

[aucahuasi]

No worries @lidavidm :)
Indeed, it makes sense to have a new jira. I can create the ticket if there is none.

[lidavidm]

I don't think there's a JIRA so please do file one.

[aucahuasi]

Here is the jira ticket for strftime, thanks @lidavidm@rok
https://issues.apache.org/jira/browse/ARROW-13916

[rok]

The Strftime ticket was merged yesterday so maybe something has changed here. Dunno.

[lidavidm]

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.

[lidavidm]

(I suppose you'd also need to change functions to templates to be able to specialize things like that...)

[aucahuasi]

Thanks @lidavidm !

I think the provided implementation (using specialization) is good enough.
Please note that this function is returning a const ref string:
const std::string& GetInputTimezone
So the compiler will throw an error if we want to return local empty strings.
And if I change the declaration to return a string (without const ref) we will be copying the timezones instead of using the refs.

Also, for the case of ISOCalendar with TimestampType we had this code:
string timezone = GetInputTimezone
and we had been performing a copy.
So I changed that to
const auto& timezone = GetInputTimezone

[lidavidm]

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.

[aucahuasi]

Sure, that's a good idea, I can try to have a single implementation.
Where should I put the empty static string? in which file?

[lidavidm]

It would go in this file, though FWIW, it would probably be easiest to just factor things into functions since these are all static methods anyways.

[aucahuasi]

@lidavidm I think this last version reuse the kernel logic and it specializes only the parts related to the timestamp logic. Please let me know if it is good enough.
TODO: I need to fix the some R tests (I'll tackle this tomorrow)

[lidavidm]

Frankly I think kernels like hour should just not work on dates - it's not meaningful.

[aucahuasi]

Make sense, pandas doesn't has that behavior too

pd.to_datetime("2019-01-03", format='%Y-%m-%d', errors='coerce').date().hour()
AttributeError: 'datetime.date'objecthasnoattribute'hour'

I'll remove the support of date32/64 for hour, min, sec ...

[lidavidm]

Instead of a bool, I think it'd be cleaner to split the date kernel registration into a separate function:

template <
template <typename...> classOp,
template <template <typename...> classOpExec, typename Duration, typename InType, typename OutType>
classExecTemplate,
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.

[aucahuasi]

Thanks, but I suggest to keep the provided implementation in this PR:

• It requires less code/It change less parts.
• It integrates well into the MakeTemporal* functions.
• It is setting up the date kernels (optionally) at the moment of creating the function.

Also, I think that if I add AddDateKernels we would need 2 versions: one for MakeTemporal and other for MakeSimpleUnaryTemporal.
So I think it makes sense to keep the current code. Let me know what do you think here @lidavidm

[lidavidm]

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);

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[aucahuasi]

Thanks, I added /*enable_date=*/ to indicate the intention of the boolean parameter.
Regarding the SimpleUnaryTemporal struct, sounds like a nice idea for a future refactor, however if you feel strongly about carrying out these sort of changes in this PR, let me know please!

[lidavidm]

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.

[lidavidm]

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);

[lidavidm]

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.

[aucahuasi]

Done, thanks @lidavidm@rok !

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[lidavidm]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case. Seems this can be changed to expect_dplyr_equal:

arrow/r/tests/testthat/test-dplyr-lubridate.R

Lines 37 to 48 in 2ef0edc

	# We can support this feature when ARROW-13138 is resolved
	test_that("date32 objects are not supported", {
	date<- ymd("2017-01-01")
	df<-tibble::tibble(date=date)
	expect_error(
	Table$create(df) %>%
	mutate(x= year(date)) %>%
	collect(),
	"Function year has no kernel matching input types"
	)
	})

[lidavidm]

Thanks!

One last thing that I forgot earlier, sorry: we should update compute.rst as well: https://github.com/apache/arrow/blob/f40856a768f2c397082da70af034080994587807/docs/source/cpp/compute.rst#temporal-component-extraction

[lidavidm]

Oh, it also seems we need to rebase here.

[aucahuasi]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case.
Seems this can be changed to expect_dplyr_equal:

@lidavidm Thank you! I fixed the issue with the R test and added some additional R tests!

[aucahuasi]

One last thing that I forgot earlier, sorry: we should update compute.rst as well:

Done!

[aucahuasi]

It seems there are some build issues on windows. Tomorrow I'll check that!

[lidavidm]

Note the TestNonexistentTimezone test is also failing on Linux.

[aucahuasi]

Note the TestNonexistentTimezone test is also failing on Linux.

Thanks, fixed!

[rok]

Looks good. Appveyor fail doesn't seem related.

It would probably be good to also add Python tests.

[lidavidm]

LGTM, thank you.

I filed https://issues.apache.org/jira/browse/ARROW-13957 for the flaky AppVeyor test.

I'm not sure if we need explicit Python tests, maybe comparing against Pandas would be nice, but unlike R we don't have higher level bindings we're trying to test.

[pitrou]

For later: either pass a const reference or a non-const pointer. Non-const references require more care from the caller.

[pitrou]

Same here: visit_value should be passed by pointer.

[lidavidm]

Thanks @pitrou, I filed ARROW-13960

[aucahuasi]

Thanks @lidavidm@rok@pitrou !

[aucahuasi]

https://issues.apache.org/jira/browse/ARROW-13138

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13138

[lidavidm]

Thanks for tackling this, this looks like a great start. I think my main questions are 1) whether we should support strftime, hour, minute, etc. on dates as I don't think they're meaningful, and 2) whether we can share the implementation of ISOCalendar instead of duplicating it.

[lidavidm]

nit: since this is for dates only, maybe have the base struct Strftime have no implementation, and implement both dates and timestamps as specializations?

[lidavidm]

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.

[aucahuasi]

I think you are right. Do you know if we have tests for casting date32/64 to strings?

[lidavidm]

We do, they're a bit thin though:

arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc

Line 1245 in 5ead375

TEST(Cast, DateToString) {

[rok]

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.

There's work to have Strftime work on timestamps without timezones (assuming UTC)
#10998. It's a philosophical discussion if it's correct or not but it will be available.

[lidavidm]

Sorry for the back-and-forth @aucahuasi. However, it might be prudent to split strftime support into a separate JIRA to minimize conflicts with the existing PR and so we can evaluate how best to share the implementation (to avoid too much code duplication).

[aucahuasi]

No worries @lidavidm :)
Indeed, it makes sense to have a new jira. I can create the ticket if there is none.

[lidavidm]

I don't think there's a JIRA so please do file one.

[aucahuasi]

Here is the jira ticket for strftime, thanks @lidavidm@rok
https://issues.apache.org/jira/browse/ARROW-13916

[rok]

The Strftime ticket was merged yesterday so maybe something has changed here. Dunno.

[lidavidm]

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.

[lidavidm]

(I suppose you'd also need to change functions to templates to be able to specialize things like that...)

[aucahuasi]

Thanks @lidavidm !

I think the provided implementation (using specialization) is good enough.
Please note that this function is returning a const ref string:
const std::string& GetInputTimezone
So the compiler will throw an error if we want to return local empty strings.
And if I change the declaration to return a string (without const ref) we will be copying the timezones instead of using the refs.

Also, for the case of ISOCalendar with TimestampType we had this code:
string timezone = GetInputTimezone
and we had been performing a copy.
So I changed that to
const auto& timezone = GetInputTimezone

[lidavidm]

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.

[aucahuasi]

Sure, that's a good idea, I can try to have a single implementation.
Where should I put the empty static string? in which file?

[lidavidm]

It would go in this file, though FWIW, it would probably be easiest to just factor things into functions since these are all static methods anyways.

[aucahuasi]

@lidavidm I think this last version reuse the kernel logic and it specializes only the parts related to the timestamp logic. Please let me know if it is good enough.
TODO: I need to fix the some R tests (I'll tackle this tomorrow)

[lidavidm]

Frankly I think kernels like hour should just not work on dates - it's not meaningful.

[aucahuasi]

Make sense, pandas doesn't has that behavior too

pd.to_datetime("2019-01-03", format='%Y-%m-%d', errors='coerce').date().hour()
AttributeError: 'datetime.date'objecthasnoattribute'hour'

I'll remove the support of date32/64 for hour, min, sec ...

[lidavidm]

Instead of a bool, I think it'd be cleaner to split the date kernel registration into a separate function:

template <
template <typename...> classOp,
template <template <typename...> classOpExec, typename Duration, typename InType, typename OutType>
classExecTemplate,
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.

[aucahuasi]

Thanks, but I suggest to keep the provided implementation in this PR:

• It requires less code/It change less parts.
• It integrates well into the MakeTemporal* functions.
• It is setting up the date kernels (optionally) at the moment of creating the function.

Also, I think that if I add AddDateKernels we would need 2 versions: one for MakeTemporal and other for MakeSimpleUnaryTemporal.
So I think it makes sense to keep the current code. Let me know what do you think here @lidavidm

[lidavidm]

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);

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[aucahuasi]

Thanks, I added /*enable_date=*/ to indicate the intention of the boolean parameter.
Regarding the SimpleUnaryTemporal struct, sounds like a nice idea for a future refactor, however if you feel strongly about carrying out these sort of changes in this PR, let me know please!

[lidavidm]

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.

[lidavidm]

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);

[lidavidm]

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.

[aucahuasi]

Done, thanks @lidavidm@rok !

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[lidavidm]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case. Seems this can be changed to expect_dplyr_equal:

arrow/r/tests/testthat/test-dplyr-lubridate.R

Lines 37 to 48 in 2ef0edc

	# We can support this feature when ARROW-13138 is resolved
	test_that("date32 objects are not supported", {
	date<- ymd("2017-01-01")
	df<-tibble::tibble(date=date)
	expect_error(
	Table$create(df) %>%
	mutate(x= year(date)) %>%
	collect(),
	"Function year has no kernel matching input types"
	)
	})

[lidavidm]

Thanks!

One last thing that I forgot earlier, sorry: we should update compute.rst as well: https://github.com/apache/arrow/blob/f40856a768f2c397082da70af034080994587807/docs/source/cpp/compute.rst#temporal-component-extraction

[lidavidm]

Oh, it also seems we need to rebase here.

[aucahuasi]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case.
Seems this can be changed to expect_dplyr_equal:

@lidavidm Thank you! I fixed the issue with the R test and added some additional R tests!

[aucahuasi]

One last thing that I forgot earlier, sorry: we should update compute.rst as well:

Done!

[aucahuasi]

It seems there are some build issues on windows. Tomorrow I'll check that!

[lidavidm]

Note the TestNonexistentTimezone test is also failing on Linux.

[aucahuasi]

Note the TestNonexistentTimezone test is also failing on Linux.

Thanks, fixed!

[rok]

Looks good. Appveyor fail doesn't seem related.

It would probably be good to also add Python tests.

[lidavidm]

LGTM, thank you.

I filed https://issues.apache.org/jira/browse/ARROW-13957 for the flaky AppVeyor test.

I'm not sure if we need explicit Python tests, maybe comparing against Pandas would be nice, but unlike R we don't have higher level bindings we're trying to test.

[pitrou]

For later: either pass a const reference or a non-const pointer. Non-const references require more care from the caller.

[pitrou]

Same here: visit_value should be passed by pointer.

[lidavidm]

Thanks @pitrou, I filed ARROW-13960

[aucahuasi]

Thanks @lidavidm@rok@pitrou !

[aucahuasi]

https://issues.apache.org/jira/browse/ARROW-13138

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13138

[lidavidm]

Thanks for tackling this, this looks like a great start. I think my main questions are 1) whether we should support strftime, hour, minute, etc. on dates as I don't think they're meaningful, and 2) whether we can share the implementation of ISOCalendar instead of duplicating it.

[lidavidm]

nit: since this is for dates only, maybe have the base struct Strftime have no implementation, and implement both dates and timestamps as specializations?

[lidavidm]

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.

[aucahuasi]

I think you are right. Do you know if we have tests for casting date32/64 to strings?

[lidavidm]

We do, they're a bit thin though:

arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc

Line 1245 in 5ead375

TEST(Cast, DateToString) {

[rok]

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.

There's work to have Strftime work on timestamps without timezones (assuming UTC)
#10998. It's a philosophical discussion if it's correct or not but it will be available.

[lidavidm]

Sorry for the back-and-forth @aucahuasi. However, it might be prudent to split strftime support into a separate JIRA to minimize conflicts with the existing PR and so we can evaluate how best to share the implementation (to avoid too much code duplication).

[aucahuasi]

No worries @lidavidm :)
Indeed, it makes sense to have a new jira. I can create the ticket if there is none.

[lidavidm]

I don't think there's a JIRA so please do file one.

[aucahuasi]

Here is the jira ticket for strftime, thanks @lidavidm@rok
https://issues.apache.org/jira/browse/ARROW-13916

[rok]

The Strftime ticket was merged yesterday so maybe something has changed here. Dunno.

[lidavidm]

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.

[lidavidm]

(I suppose you'd also need to change functions to templates to be able to specialize things like that...)

[aucahuasi]

Thanks @lidavidm !

I think the provided implementation (using specialization) is good enough.
Please note that this function is returning a const ref string:
const std::string& GetInputTimezone
So the compiler will throw an error if we want to return local empty strings.
And if I change the declaration to return a string (without const ref) we will be copying the timezones instead of using the refs.

Also, for the case of ISOCalendar with TimestampType we had this code:
string timezone = GetInputTimezone
and we had been performing a copy.
So I changed that to
const auto& timezone = GetInputTimezone

[lidavidm]

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.

[aucahuasi]

Sure, that's a good idea, I can try to have a single implementation.
Where should I put the empty static string? in which file?

[lidavidm]

It would go in this file, though FWIW, it would probably be easiest to just factor things into functions since these are all static methods anyways.

[aucahuasi]

@lidavidm I think this last version reuse the kernel logic and it specializes only the parts related to the timestamp logic. Please let me know if it is good enough.
TODO: I need to fix the some R tests (I'll tackle this tomorrow)

[lidavidm]

Frankly I think kernels like hour should just not work on dates - it's not meaningful.

[aucahuasi]

Make sense, pandas doesn't has that behavior too

pd.to_datetime("2019-01-03", format='%Y-%m-%d', errors='coerce').date().hour()
AttributeError: 'datetime.date'objecthasnoattribute'hour'

I'll remove the support of date32/64 for hour, min, sec ...

[lidavidm]

Instead of a bool, I think it'd be cleaner to split the date kernel registration into a separate function:

template <
template <typename...> classOp,
template <template <typename...> classOpExec, typename Duration, typename InType, typename OutType>
classExecTemplate,
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.

[aucahuasi]

Thanks, but I suggest to keep the provided implementation in this PR:

• It requires less code/It change less parts.
• It integrates well into the MakeTemporal* functions.
• It is setting up the date kernels (optionally) at the moment of creating the function.

Also, I think that if I add AddDateKernels we would need 2 versions: one for MakeTemporal and other for MakeSimpleUnaryTemporal.
So I think it makes sense to keep the current code. Let me know what do you think here @lidavidm

[lidavidm]

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);

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[aucahuasi]

Thanks, I added /*enable_date=*/ to indicate the intention of the boolean parameter.
Regarding the SimpleUnaryTemporal struct, sounds like a nice idea for a future refactor, however if you feel strongly about carrying out these sort of changes in this PR, let me know please!

[lidavidm]

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.

[lidavidm]

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);

[lidavidm]

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.

[aucahuasi]

Done, thanks @lidavidm@rok !

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[lidavidm]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case. Seems this can be changed to expect_dplyr_equal:

arrow/r/tests/testthat/test-dplyr-lubridate.R

Lines 37 to 48 in 2ef0edc

	# We can support this feature when ARROW-13138 is resolved
	test_that("date32 objects are not supported", {
	date<- ymd("2017-01-01")
	df<-tibble::tibble(date=date)
	expect_error(
	Table$create(df) %>%
	mutate(x= year(date)) %>%
	collect(),
	"Function year has no kernel matching input types"
	)
	})

[lidavidm]

Thanks!

One last thing that I forgot earlier, sorry: we should update compute.rst as well: https://github.com/apache/arrow/blob/f40856a768f2c397082da70af034080994587807/docs/source/cpp/compute.rst#temporal-component-extraction

[lidavidm]

Oh, it also seems we need to rebase here.

[aucahuasi]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case.
Seems this can be changed to expect_dplyr_equal:

@lidavidm Thank you! I fixed the issue with the R test and added some additional R tests!

[aucahuasi]

One last thing that I forgot earlier, sorry: we should update compute.rst as well:

Done!

[aucahuasi]

It seems there are some build issues on windows. Tomorrow I'll check that!

[lidavidm]

Note the TestNonexistentTimezone test is also failing on Linux.

[aucahuasi]

Note the TestNonexistentTimezone test is also failing on Linux.

Thanks, fixed!

[rok]

Looks good. Appveyor fail doesn't seem related.

It would probably be good to also add Python tests.

[lidavidm]

LGTM, thank you.

I filed https://issues.apache.org/jira/browse/ARROW-13957 for the flaky AppVeyor test.

I'm not sure if we need explicit Python tests, maybe comparing against Pandas would be nice, but unlike R we don't have higher level bindings we're trying to test.

[pitrou]

For later: either pass a const reference or a non-const pointer. Non-const references require more care from the caller.

[pitrou]

Same here: visit_value should be passed by pointer.

[lidavidm]

Thanks @pitrou, I filed ARROW-13960

[aucahuasi]

Thanks @lidavidm@rok@pitrou !

[aucahuasi]

https://issues.apache.org/jira/browse/ARROW-13138

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13138

[lidavidm]

Thanks for tackling this, this looks like a great start. I think my main questions are 1) whether we should support strftime, hour, minute, etc. on dates as I don't think they're meaningful, and 2) whether we can share the implementation of ISOCalendar instead of duplicating it.

[lidavidm]

nit: since this is for dates only, maybe have the base struct Strftime have no implementation, and implement both dates and timestamps as specializations?

[lidavidm]

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.

[aucahuasi]

I think you are right. Do you know if we have tests for casting date32/64 to strings?

[lidavidm]

We do, they're a bit thin though:

arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc

Line 1245 in 5ead375

TEST(Cast, DateToString) {

[rok]

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.

There's work to have Strftime work on timestamps without timezones (assuming UTC)
#10998. It's a philosophical discussion if it's correct or not but it will be available.

[lidavidm]

Sorry for the back-and-forth @aucahuasi. However, it might be prudent to split strftime support into a separate JIRA to minimize conflicts with the existing PR and so we can evaluate how best to share the implementation (to avoid too much code duplication).

[aucahuasi]

No worries @lidavidm :)
Indeed, it makes sense to have a new jira. I can create the ticket if there is none.

[lidavidm]

I don't think there's a JIRA so please do file one.

[aucahuasi]

Here is the jira ticket for strftime, thanks @lidavidm@rok
https://issues.apache.org/jira/browse/ARROW-13916

[rok]

The Strftime ticket was merged yesterday so maybe something has changed here. Dunno.

[lidavidm]

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.

[lidavidm]

(I suppose you'd also need to change functions to templates to be able to specialize things like that...)

[aucahuasi]

Thanks @lidavidm !

I think the provided implementation (using specialization) is good enough.
Please note that this function is returning a const ref string:
const std::string& GetInputTimezone
So the compiler will throw an error if we want to return local empty strings.
And if I change the declaration to return a string (without const ref) we will be copying the timezones instead of using the refs.

Also, for the case of ISOCalendar with TimestampType we had this code:
string timezone = GetInputTimezone
and we had been performing a copy.
So I changed that to
const auto& timezone = GetInputTimezone

[lidavidm]

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.

[aucahuasi]

Sure, that's a good idea, I can try to have a single implementation.
Where should I put the empty static string? in which file?

[lidavidm]

It would go in this file, though FWIW, it would probably be easiest to just factor things into functions since these are all static methods anyways.

[aucahuasi]

@lidavidm I think this last version reuse the kernel logic and it specializes only the parts related to the timestamp logic. Please let me know if it is good enough.
TODO: I need to fix the some R tests (I'll tackle this tomorrow)

[lidavidm]

Frankly I think kernels like hour should just not work on dates - it's not meaningful.

[aucahuasi]

Make sense, pandas doesn't has that behavior too

pd.to_datetime("2019-01-03", format='%Y-%m-%d', errors='coerce').date().hour()
AttributeError: 'datetime.date'objecthasnoattribute'hour'

I'll remove the support of date32/64 for hour, min, sec ...

[lidavidm]

Instead of a bool, I think it'd be cleaner to split the date kernel registration into a separate function:

template <
template <typename...> classOp,
template <template <typename...> classOpExec, typename Duration, typename InType, typename OutType>
classExecTemplate,
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.

[aucahuasi]

Thanks, but I suggest to keep the provided implementation in this PR:

• It requires less code/It change less parts.
• It integrates well into the MakeTemporal* functions.
• It is setting up the date kernels (optionally) at the moment of creating the function.

Also, I think that if I add AddDateKernels we would need 2 versions: one for MakeTemporal and other for MakeSimpleUnaryTemporal.
So I think it makes sense to keep the current code. Let me know what do you think here @lidavidm

[lidavidm]

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);

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[aucahuasi]

Thanks, I added /*enable_date=*/ to indicate the intention of the boolean parameter.
Regarding the SimpleUnaryTemporal struct, sounds like a nice idea for a future refactor, however if you feel strongly about carrying out these sort of changes in this PR, let me know please!

[lidavidm]

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.

[lidavidm]

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);

[lidavidm]

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.

[aucahuasi]

Done, thanks @lidavidm@rok !

[lidavidm]

I will note though, a struct like this can collapse MakeTemporal and MakeSimpleUnaryTemporal into one function:

template <template <typename...> classOp, typename Duration, typename InType>
structSimpleUnaryTemporal {
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.

[lidavidm]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case. Seems this can be changed to expect_dplyr_equal:

arrow/r/tests/testthat/test-dplyr-lubridate.R

Lines 37 to 48 in 2ef0edc

	# We can support this feature when ARROW-13138 is resolved
	test_that("date32 objects are not supported", {
	date<- ymd("2017-01-01")
	df<-tibble::tibble(date=date)
	expect_error(
	Table$create(df) %>%
	mutate(x= year(date)) %>%
	collect(),
	"Function year has no kernel matching input types"
	)
	})

[lidavidm]

Thanks!

One last thing that I forgot earlier, sorry: we should update compute.rst as well: https://github.com/apache/arrow/blob/f40856a768f2c397082da70af034080994587807/docs/source/cpp/compute.rst#temporal-component-extraction

[lidavidm]

Oh, it also seems we need to rebase here.

[aucahuasi]

For the failed R test, it looks like it expected year(date) to fail but of course that's no longer the case.
Seems this can be changed to expect_dplyr_equal:

@lidavidm Thank you! I fixed the issue with the R test and added some additional R tests!

[aucahuasi]

One last thing that I forgot earlier, sorry: we should update compute.rst as well:

Done!

[aucahuasi]

It seems there are some build issues on windows. Tomorrow I'll check that!

[lidavidm]

Note the TestNonexistentTimezone test is also failing on Linux.

[aucahuasi]

Note the TestNonexistentTimezone test is also failing on Linux.

Thanks, fixed!

[rok]

Looks good. Appveyor fail doesn't seem related.

It would probably be good to also add Python tests.

[lidavidm]

LGTM, thank you.

I filed https://issues.apache.org/jira/browse/ARROW-13957 for the flaky AppVeyor test.

I'm not sure if we need explicit Python tests, maybe comparing against Pandas would be nice, but unlike R we don't have higher level bindings we're trying to test.

[pitrou]

For later: either pass a const reference or a non-const pointer. Non-const references require more care from the caller.

[pitrou]

Same here: visit_value should be passed by pointer.

[lidavidm]

Thanks @pitrou, I filed ARROW-13960

[aucahuasi]

Thanks @lidavidm@rok@pitrou !