jonkeane commented on pull request #12357: URL: https://github.com/apache/arrow/pull/12357#issuecomment-1035575913
> Worth noting that the current implementation diverges from lubridate::tz() as indicated in the conversation, namely it does not return "UTC" for strings, numerics or NAs. I think this is totally fine. For at least the numerics, lubridate will soon be doing what we do. For the others (strings, and bare NAs) it doesn't look like they are going to deprecate them, but I don't think we need to also follow that. We should note that (at least in a comment in the code, or in our documentation for the bindings if there's a place to put it). > NA will be "contagious" in arrow's tz() binding (as opposed to lubridate::tz() where they aren't): Yeah, we have a choice here. I'm actually a bit surprised that `lubridate::tz()` returns a timezone that's not `UTC` there given the docs(!). But actually that behavior seems right. A bare `NA` either shouldn't produce a timezone (or produce a default UTC for backwards compatibility). But if the NA has a timezone attribute, that should be used. Given that, we probably should take out the `is.na()` bit in our binding since that will match what `lubridate::tz()` here. I only suggested adding it if we were aiming to mimic the `lubridate::tz(NA)` behavior here (which I thought would be `UTC`, but turns out, it's not!). That has the added benefit that we don't do by-element comparison, which is nice! -- 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]
