fix(rust, python): respect time_zone in lazy date_range #8591
Conversation
github-actions
bot
added
fix
| DataType::Datetime(_, _) | DataType::Time => { | ||
| DataType::Datetime(tu, ref tz) => { |
There was a problem hiding this comment.
separating the Datetime and Time arms to make use of tu and tz from Datetime
| DateRange { .. } => return mapper.map_to_supertype(), | ||
| DateRange { |
There was a problem hiding this comment.
the output dtype may change according to tz, so need to do some extra computation here
There was a problem hiding this comment.
Right, can you add that as comment?
|
|
||
| let (tu, tz) = match dtype { |
There was a problem hiding this comment.
the output dtype isn't necessarily start.dtype() (if tz is not None). so instead of just recording tu and tz, I'm assigning to dtype, which contains tu and tz.
Then,
- for
Date, I just useMillisecondsastuandNonefortz - for
Time, same, but withNanosecondsastu - for
Datetime, use itstuandtz
| start | ||
| .datetime() | ||
| .unwrap() | ||
| .replace_time_zone(None, None)? |
There was a problem hiding this comment.
if start and stop have a time zone, I'm removing it, as date_range_impl deals with that
MarcoGorelli
requested review from
ritchie46,
stinodego and
alexander-beedie
as code owners
| closed: _, | ||
| tz, | ||
| } => { | ||
| let mut ret = mapper.map_to_supertype()?; |
There was a problem hiding this comment.
Can we hoist this into a function. That keeps the schema branch a bit more lean.
There was a problem hiding this comment.
yup! by doing that, I realised that I hadn't quite done it correctly, and that it was wrong for (lazy) time_range too #9036
+1 for smaller functions and leaner logic, thanks!
| DateRange { .. } => return mapper.map_to_supertype(), | ||
| DateRange { |
There was a problem hiding this comment.
Right, can you add that as comment?
|
Thanks for the reminder. Forgot about this one. |
closes #8512
marking as draft while I split part of it out into a separate pr