rok commented on pull request #10998:
URL: https://github.com/apache/arrow/pull/10998#issuecomment-905796113


   > > 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)](https://github.com/apache/arrow/pull/10647#issuecomment-901783928) 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to