ARROW-13684: [C++][Compute] Strftime kernel follow-up

[rok]

This is to resolve ARROW-13684.

1. Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?
2. Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?
3. Document %S behavior. What would be a good location to do that?

[rok]

@jorisvandenbossche

[github-actions]

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

[jorisvandenbossche]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

[rok]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Right. And for the option where we would have UTC timestamps having "Z" at the end would require something like:

if (timezone == "UTC" && self.options.format == "") {
self.options.format = "%Y-%m-%dT%H:%M:%SZ";
} else {
self.options.format = "%Y-%m-%dT%H:%M:%S";
}

Where we would have self.options.format = "" by default.
I'm not sure this is really worth it. I'd keep the timezone out of the default format.

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Ok, let's return invalid when detecting %z/%Z and a timezone-less timestamp then?

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

Sounds good.

[rok]

@jorisvandenbossche I've pushed changes for second and third (printing timezoneless timestamps and docs) point. I left the first as was (not timezone data by default).
Please see if this looks ok.

[westonpace]

Some minor wording nits but overall the change seems good.

[westonpace]

Nit: I don't know if cannot print is quite right. The user might be running strftime to store or display on a GUI or send to some other tool. Perhaps cannot convert to string? Or cannot format?

[rok]

Yeah, good point. I went with your first proposal.

[westonpace]

This comment seems to suggest that only microsecond and second are supported. I like the wording down below which makes it more clear that seconds through nanoseconds are supported.

[rok]

Indeed. Changed the language. Could you check if it's good now?

[westonpace]

Very clear. Thanks.

[rok]

Thanks for the comments @westonpace !

[rok]

@jorisvandenbossche@westonpace I've made a minor change in the text (decimal points -> decimal places).
Other than that I think this is done.

[jorisvandenbossche]

Thanks for the updates!

[jorisvandenbossche]

I would also add something about "To obtain integer seconds, cast to timestamp with second resolution" to be explicit about the workaround.

[rok]

Done.

[jorisvandenbossche]

This is no longer up to date

[rok]

It seems error description is outdated for all extract kernels. Thanks for pointing it out! Fixing.

[jorisvandenbossche]

I don't think this is accurate? It doesn't depend on the number of seconds present, but solely on the unit of the timestamp type?

[jorisvandenbossche]

It seems you used some explanation about this from https://howardhinnant.github.io/date/date.html#to_stream_formatting, but I would maybe use the same text as you added above in strftime_doc, as I think that's clearer / more relevant from Arrow's perspective.

[rok]

Makes sense yeah. Done.

[rok]

Thanks for the review @jorisvandenbossche@pitrou.
I addressed the comments and pushed the changes.

[pitrou]

Thanks for the updates @rok. I will merge now if CI is green.

[rok]

Thanks @westonpace@jorisvandenbossche@pitrou !

[rok]

This is to resolve ARROW-13684.

1. Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?
2. Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?
3. Document %S behavior. What would be a good location to do that?

[rok]

@jorisvandenbossche

[github-actions]

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

[jorisvandenbossche]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

[rok]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Right. And for the option where we would have UTC timestamps having "Z" at the end would require something like:

if (timezone == "UTC" && self.options.format == "") {
self.options.format = "%Y-%m-%dT%H:%M:%SZ";
} else {
self.options.format = "%Y-%m-%dT%H:%M:%S";
}

Where we would have self.options.format = "" by default.
I'm not sure this is really worth it. I'd keep the timezone out of the default format.

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Ok, let's return invalid when detecting %z/%Z and a timezone-less timestamp then?

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

Sounds good.

[rok]

@jorisvandenbossche I've pushed changes for second and third (printing timezoneless timestamps and docs) point. I left the first as was (not timezone data by default).
Please see if this looks ok.

[westonpace]

Some minor wording nits but overall the change seems good.

[westonpace]

Nit: I don't know if cannot print is quite right. The user might be running strftime to store or display on a GUI or send to some other tool. Perhaps cannot convert to string? Or cannot format?

[rok]

Yeah, good point. I went with your first proposal.

[westonpace]

This comment seems to suggest that only microsecond and second are supported. I like the wording down below which makes it more clear that seconds through nanoseconds are supported.

[rok]

Indeed. Changed the language. Could you check if it's good now?

[westonpace]

Very clear. Thanks.

[rok]

Thanks for the comments @westonpace !

[rok]

@jorisvandenbossche@westonpace I've made a minor change in the text (decimal points -> decimal places).
Other than that I think this is done.

[jorisvandenbossche]

Thanks for the updates!

[jorisvandenbossche]

I would also add something about "To obtain integer seconds, cast to timestamp with second resolution" to be explicit about the workaround.

[rok]

Done.

[jorisvandenbossche]

This is no longer up to date

[rok]

It seems error description is outdated for all extract kernels. Thanks for pointing it out! Fixing.

[jorisvandenbossche]

I don't think this is accurate? It doesn't depend on the number of seconds present, but solely on the unit of the timestamp type?

[jorisvandenbossche]

It seems you used some explanation about this from https://howardhinnant.github.io/date/date.html#to_stream_formatting, but I would maybe use the same text as you added above in strftime_doc, as I think that's clearer / more relevant from Arrow's perspective.

[rok]

Makes sense yeah. Done.

[rok]

Thanks for the review @jorisvandenbossche@pitrou.
I addressed the comments and pushed the changes.

[pitrou]

Thanks for the updates @rok. I will merge now if CI is green.

[rok]

Thanks @westonpace@jorisvandenbossche@pitrou !

[rok]

This is to resolve ARROW-13684.

1. Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?
2. Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?
3. Document %S behavior. What would be a good location to do that?

[rok]

@jorisvandenbossche

[github-actions]

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

[jorisvandenbossche]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

[rok]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Right. And for the option where we would have UTC timestamps having "Z" at the end would require something like:

if (timezone == "UTC" && self.options.format == "") {
self.options.format = "%Y-%m-%dT%H:%M:%SZ";
} else {
self.options.format = "%Y-%m-%dT%H:%M:%S";
}

Where we would have self.options.format = "" by default.
I'm not sure this is really worth it. I'd keep the timezone out of the default format.

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Ok, let's return invalid when detecting %z/%Z and a timezone-less timestamp then?

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

Sounds good.

[rok]

@jorisvandenbossche I've pushed changes for second and third (printing timezoneless timestamps and docs) point. I left the first as was (not timezone data by default).
Please see if this looks ok.

[westonpace]

Some minor wording nits but overall the change seems good.

[westonpace]

Nit: I don't know if cannot print is quite right. The user might be running strftime to store or display on a GUI or send to some other tool. Perhaps cannot convert to string? Or cannot format?

[rok]

Yeah, good point. I went with your first proposal.

[westonpace]

This comment seems to suggest that only microsecond and second are supported. I like the wording down below which makes it more clear that seconds through nanoseconds are supported.

[rok]

Indeed. Changed the language. Could you check if it's good now?

[westonpace]

Very clear. Thanks.

[rok]

Thanks for the comments @westonpace !

[rok]

@jorisvandenbossche@westonpace I've made a minor change in the text (decimal points -> decimal places).
Other than that I think this is done.

[jorisvandenbossche]

Thanks for the updates!

[jorisvandenbossche]

I would also add something about "To obtain integer seconds, cast to timestamp with second resolution" to be explicit about the workaround.

[rok]

Done.

[jorisvandenbossche]

This is no longer up to date

[rok]

It seems error description is outdated for all extract kernels. Thanks for pointing it out! Fixing.

[jorisvandenbossche]

I don't think this is accurate? It doesn't depend on the number of seconds present, but solely on the unit of the timestamp type?

[jorisvandenbossche]

It seems you used some explanation about this from https://howardhinnant.github.io/date/date.html#to_stream_formatting, but I would maybe use the same text as you added above in strftime_doc, as I think that's clearer / more relevant from Arrow's perspective.

[rok]

Makes sense yeah. Done.

[rok]

Thanks for the review @jorisvandenbossche@pitrou.
I addressed the comments and pushed the changes.

[pitrou]

Thanks for the updates @rok. I will merge now if CI is green.

[rok]

Thanks @westonpace@jorisvandenbossche@pitrou !

[rok]

This is to resolve ARROW-13684.

1. Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?
2. Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?
3. Document %S behavior. What would be a good location to do that?

[rok]

@jorisvandenbossche

[github-actions]

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

[jorisvandenbossche]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

[rok]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Right. And for the option where we would have UTC timestamps having "Z" at the end would require something like:

if (timezone == "UTC" && self.options.format == "") {
self.options.format = "%Y-%m-%dT%H:%M:%SZ";
} else {
self.options.format = "%Y-%m-%dT%H:%M:%S";
}

Where we would have self.options.format = "" by default.
I'm not sure this is really worth it. I'd keep the timezone out of the default format.

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Ok, let's return invalid when detecting %z/%Z and a timezone-less timestamp then?

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

Sounds good.

[rok]

@jorisvandenbossche I've pushed changes for second and third (printing timezoneless timestamps and docs) point. I left the first as was (not timezone data by default).
Please see if this looks ok.

[westonpace]

Some minor wording nits but overall the change seems good.

[westonpace]

Nit: I don't know if cannot print is quite right. The user might be running strftime to store or display on a GUI or send to some other tool. Perhaps cannot convert to string? Or cannot format?

[rok]

Yeah, good point. I went with your first proposal.

[westonpace]

This comment seems to suggest that only microsecond and second are supported. I like the wording down below which makes it more clear that seconds through nanoseconds are supported.

[rok]

Indeed. Changed the language. Could you check if it's good now?

[westonpace]

Very clear. Thanks.

[rok]

Thanks for the comments @westonpace !

[rok]

@jorisvandenbossche@westonpace I've made a minor change in the text (decimal points -> decimal places).
Other than that I think this is done.

[jorisvandenbossche]

Thanks for the updates!

[jorisvandenbossche]

I would also add something about "To obtain integer seconds, cast to timestamp with second resolution" to be explicit about the workaround.

[rok]

Done.

[jorisvandenbossche]

This is no longer up to date

[rok]

It seems error description is outdated for all extract kernels. Thanks for pointing it out! Fixing.

[jorisvandenbossche]

I don't think this is accurate? It doesn't depend on the number of seconds present, but solely on the unit of the timestamp type?

[jorisvandenbossche]

It seems you used some explanation about this from https://howardhinnant.github.io/date/date.html#to_stream_formatting, but I would maybe use the same text as you added above in strftime_doc, as I think that's clearer / more relevant from Arrow's perspective.

[rok]

Makes sense yeah. Done.

[rok]

Thanks for the review @jorisvandenbossche@pitrou.
I addressed the comments and pushed the changes.

[pitrou]

Thanks for the updates @rok. I will merge now if CI is green.

[rok]

Thanks @westonpace@jorisvandenbossche@pitrou !

[rok]

This is to resolve ARROW-13684.

1. Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?
2. Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?
3. Document %S behavior. What would be a good location to do that?

[rok]

@jorisvandenbossche

[github-actions]

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

[jorisvandenbossche]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

[rok]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Right. And for the option where we would have UTC timestamps having "Z" at the end would require something like:

if (timezone == "UTC" && self.options.format == "") {
self.options.format = "%Y-%m-%dT%H:%M:%SZ";
} else {
self.options.format = "%Y-%m-%dT%H:%M:%S";
}

Where we would have self.options.format = "" by default.
I'm not sure this is really worth it. I'd keep the timezone out of the default format.

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Ok, let's return invalid when detecting %z/%Z and a timezone-less timestamp then?

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

Sounds good.

[rok]

@jorisvandenbossche I've pushed changes for second and third (printing timezoneless timestamps and docs) point. I left the first as was (not timezone data by default).
Please see if this looks ok.

[westonpace]

Some minor wording nits but overall the change seems good.

[westonpace]

Nit: I don't know if cannot print is quite right. The user might be running strftime to store or display on a GUI or send to some other tool. Perhaps cannot convert to string? Or cannot format?

[rok]

Yeah, good point. I went with your first proposal.

[westonpace]

This comment seems to suggest that only microsecond and second are supported. I like the wording down below which makes it more clear that seconds through nanoseconds are supported.

[rok]

Indeed. Changed the language. Could you check if it's good now?

[westonpace]

Very clear. Thanks.

[rok]

Thanks for the comments @westonpace !

[rok]

@jorisvandenbossche@westonpace I've made a minor change in the text (decimal points -> decimal places).
Other than that I think this is done.

[jorisvandenbossche]

Thanks for the updates!

[jorisvandenbossche]

I would also add something about "To obtain integer seconds, cast to timestamp with second resolution" to be explicit about the workaround.

[rok]

Done.

[jorisvandenbossche]

This is no longer up to date

[rok]

It seems error description is outdated for all extract kernels. Thanks for pointing it out! Fixing.

[jorisvandenbossche]

I don't think this is accurate? It doesn't depend on the number of seconds present, but solely on the unit of the timestamp type?

[jorisvandenbossche]

It seems you used some explanation about this from https://howardhinnant.github.io/date/date.html#to_stream_formatting, but I would maybe use the same text as you added above in strftime_doc, as I think that's clearer / more relevant from Arrow's perspective.

[rok]

Makes sense yeah. Done.

[rok]

Thanks for the review @jorisvandenbossche@pitrou.
I addressed the comments and pushed the changes.

[pitrou]

Thanks for the updates @rok. I will merge now if CI is green.

[rok]

Thanks @westonpace@jorisvandenbossche@pitrou !

[rok]

This is to resolve ARROW-13684.

1. Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?
2. Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?
3. Document %S behavior. What would be a good location to do that?

[rok]

@jorisvandenbossche

[github-actions]

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

[jorisvandenbossche]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

[rok]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Right. And for the option where we would have UTC timestamps having "Z" at the end would require something like:

if (timezone == "UTC" && self.options.format == "") {
self.options.format = "%Y-%m-%dT%H:%M:%SZ";
} else {
self.options.format = "%Y-%m-%dT%H:%M:%S";
}

Where we would have self.options.format = "" by default.
I'm not sure this is really worth it. I'd keep the timezone out of the default format.

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Ok, let's return invalid when detecting %z/%Z and a timezone-less timestamp then?

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

Sounds good.

[rok]

@jorisvandenbossche I've pushed changes for second and third (printing timezoneless timestamps and docs) point. I left the first as was (not timezone data by default).
Please see if this looks ok.

[westonpace]

Some minor wording nits but overall the change seems good.

[westonpace]

Nit: I don't know if cannot print is quite right. The user might be running strftime to store or display on a GUI or send to some other tool. Perhaps cannot convert to string? Or cannot format?

[rok]

Yeah, good point. I went with your first proposal.

[westonpace]

This comment seems to suggest that only microsecond and second are supported. I like the wording down below which makes it more clear that seconds through nanoseconds are supported.

[rok]

Indeed. Changed the language. Could you check if it's good now?

[westonpace]

Very clear. Thanks.

[rok]

Thanks for the comments @westonpace !

[rok]

@jorisvandenbossche@westonpace I've made a minor change in the text (decimal points -> decimal places).
Other than that I think this is done.

[jorisvandenbossche]

Thanks for the updates!

[jorisvandenbossche]

I would also add something about "To obtain integer seconds, cast to timestamp with second resolution" to be explicit about the workaround.

[rok]

Done.

[jorisvandenbossche]

This is no longer up to date

[rok]

It seems error description is outdated for all extract kernels. Thanks for pointing it out! Fixing.

[jorisvandenbossche]

I don't think this is accurate? It doesn't depend on the number of seconds present, but solely on the unit of the timestamp type?

[jorisvandenbossche]

It seems you used some explanation about this from https://howardhinnant.github.io/date/date.html#to_stream_formatting, but I would maybe use the same text as you added above in strftime_doc, as I think that's clearer / more relevant from Arrow's perspective.

[rok]

Makes sense yeah. Done.

[rok]

Thanks for the review @jorisvandenbossche@pitrou.
I addressed the comments and pushed the changes.

[pitrou]

Thanks for the updates @rok. I will merge now if CI is green.

[rok]

Thanks @westonpace@jorisvandenbossche@pitrou !

[rok]

This is to resolve ARROW-13684.

1. Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?
2. Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?
3. Document %S behavior. What would be a good location to do that?

[rok]

@jorisvandenbossche

[github-actions]

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

[jorisvandenbossche]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

[rok]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Right. And for the option where we would have UTC timestamps having "Z" at the end would require something like:

if (timezone == "UTC" && self.options.format == "") {
self.options.format = "%Y-%m-%dT%H:%M:%SZ";
} else {
self.options.format = "%Y-%m-%dT%H:%M:%S";
}

Where we would have self.options.format = "" by default.
I'm not sure this is really worth it. I'd keep the timezone out of the default format.

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Ok, let's return invalid when detecting %z/%Z and a timezone-less timestamp then?

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

Sounds good.

[rok]

@jorisvandenbossche I've pushed changes for second and third (printing timezoneless timestamps and docs) point. I left the first as was (not timezone data by default).
Please see if this looks ok.

[westonpace]

Some minor wording nits but overall the change seems good.

[westonpace]

Nit: I don't know if cannot print is quite right. The user might be running strftime to store or display on a GUI or send to some other tool. Perhaps cannot convert to string? Or cannot format?

[rok]

Yeah, good point. I went with your first proposal.

[westonpace]

This comment seems to suggest that only microsecond and second are supported. I like the wording down below which makes it more clear that seconds through nanoseconds are supported.

[rok]

Indeed. Changed the language. Could you check if it's good now?

[westonpace]

Very clear. Thanks.

[rok]

Thanks for the comments @westonpace !

[rok]

@jorisvandenbossche@westonpace I've made a minor change in the text (decimal points -> decimal places).
Other than that I think this is done.

[jorisvandenbossche]

Thanks for the updates!

[jorisvandenbossche]

I would also add something about "To obtain integer seconds, cast to timestamp with second resolution" to be explicit about the workaround.

[rok]

Done.

[jorisvandenbossche]

This is no longer up to date

[rok]

It seems error description is outdated for all extract kernels. Thanks for pointing it out! Fixing.

[jorisvandenbossche]

I don't think this is accurate? It doesn't depend on the number of seconds present, but solely on the unit of the timestamp type?

[jorisvandenbossche]

It seems you used some explanation about this from https://howardhinnant.github.io/date/date.html#to_stream_formatting, but I would maybe use the same text as you added above in strftime_doc, as I think that's clearer / more relevant from Arrow's perspective.

[rok]

Makes sense yeah. Done.

[rok]

Thanks for the review @jorisvandenbossche@pitrou.
I addressed the comments and pushed the changes.

[pitrou]

Thanks for the updates @rok. I will merge now if CI is green.

[rok]

Thanks @westonpace@jorisvandenbossche@pitrou !

[rok]

This is to resolve ARROW-13684.

1. Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?
2. Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?
3. Document %S behavior. What would be a good location to do that?

[rok]

@jorisvandenbossche

[github-actions]

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

[jorisvandenbossche]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

[rok]

Default strftime string is now %Y-%m-%dT%H:%M:%S. Perhaps %Y-%m-%dT%H:%M:%S%z would be better?

Not fully sure about this one. In #10647 (comment) I mentioned the options of showing no timezone information (what you did now), include a numeric offset (so adding %z) or erroring. And for UTC it's also an option to use the literal "Z".

Including a numeric offset of using literal Z (for UTC) both mean that the default format depends on the type (because for timestamp without timezone we shouldn't add an offset).

Right. And for the option where we would have UTC timestamps having "Z" at the end would require something like:

if (timezone == "UTC" && self.options.format == "") {
self.options.format = "%Y-%m-%dT%H:%M:%SZ";
} else {
self.options.format = "%Y-%m-%dT%H:%M:%S";
}

Where we would have self.options.format = "" by default.
I'm not sure this is really worth it. I'd keep the timezone out of the default format.

Timestamps without timezone are now strftime-ed as if they were in UTC. Not sure this is the way to go. What if the local time is really invalid but we can't tell?

Timestamps without timezone should be formatted as is, showing the wall clock time they represent. So I think formatting as if they are in UTC but without any timezone indication is correct. Although we should not allow adding timezone information with %Z or %z in those cases.
I don't think we need to care about invalid local times when printing, that's a user responsibility (and anyway, you only know if a time is invalid once you assume a certain timezone, which we don't do in strftime).

Ok, let's return invalid when detecting %z/%Z and a timezone-less timestamp then?

Document %S behavior. What would be a good location to do that?

You could start with the compute.rst table notes, but personally I would also include it in the docstring.

Sounds good.

[rok]

@jorisvandenbossche I've pushed changes for second and third (printing timezoneless timestamps and docs) point. I left the first as was (not timezone data by default).
Please see if this looks ok.

[westonpace]

Some minor wording nits but overall the change seems good.

[westonpace]

Nit: I don't know if cannot print is quite right. The user might be running strftime to store or display on a GUI or send to some other tool. Perhaps cannot convert to string? Or cannot format?

[rok]

Yeah, good point. I went with your first proposal.

[westonpace]

This comment seems to suggest that only microsecond and second are supported. I like the wording down below which makes it more clear that seconds through nanoseconds are supported.

[rok]

Indeed. Changed the language. Could you check if it's good now?

[westonpace]

Very clear. Thanks.

[rok]

Thanks for the comments @westonpace !

[rok]

@jorisvandenbossche@westonpace I've made a minor change in the text (decimal points -> decimal places).
Other than that I think this is done.

[jorisvandenbossche]

Thanks for the updates!

[jorisvandenbossche]

I would also add something about "To obtain integer seconds, cast to timestamp with second resolution" to be explicit about the workaround.

[rok]

Done.

[jorisvandenbossche]

This is no longer up to date

[rok]

It seems error description is outdated for all extract kernels. Thanks for pointing it out! Fixing.

[jorisvandenbossche]

I don't think this is accurate? It doesn't depend on the number of seconds present, but solely on the unit of the timestamp type?

[jorisvandenbossche]

It seems you used some explanation about this from https://howardhinnant.github.io/date/date.html#to_stream_formatting, but I would maybe use the same text as you added above in strftime_doc, as I think that's clearer / more relevant from Arrow's perspective.

[rok]

Makes sense yeah. Done.

[rok]

Thanks for the review @jorisvandenbossche@pitrou.
I addressed the comments and pushed the changes.

[pitrou]

Thanks for the updates @rok. I will merge now if CI is green.

[rok]

Thanks @westonpace@jorisvandenbossche@pitrou !