API, Core: Move micros and days conversions to DateTimeUtil #6199
Conversation
| case DAYS: | ||
| return days; | ||
| default: | ||
| throw new UnsupportedOperationException("Unsupported time unit: " + granularity); |
There was a problem hiding this comment.
This block is now similar to toHumanString below.
|
@kbendick @rdblue @RussellSpitzer @szehon-ho @flyrain, could you take a look at this PR? I need to use these conversions in our Spark function catalog. |
| return convertMicros(micros, ChronoUnit.HOURS); | ||
| } | ||
|
|
||
| private static int convertMicros(long micros, ChronoUnit granularity) { |
There was a problem hiding this comment.
These conversions are covered by TestDates and TestTimestamps suites.
nastra
left a comment
nastra
left a comment
There was a problem hiding this comment.
LGTM, I think it would be good to also move TestDateTimeUtil to api
szehon-ho
left a comment
szehon-ho
left a comment
There was a problem hiding this comment.
Yea, would be good to get more eyes on it to double check, but otherwise looks like a clean refactor to me.
|
|
||
| private static int convertMicros(long micros, ChronoUnit granularity) { | ||
| if (micros >= 0) { | ||
| long epochSecond = Math.floorDiv(micros, MICROS_PER_SECOND); |
There was a problem hiding this comment.
Nit: am I missing something or can we just pull these out of individual blocks?
There was a problem hiding this comment.
I think we can pull only epochSecond so I wasn't sure it was worth it. Let me do that.
There was a problem hiding this comment.
I moved that var but it became somehow disconnected from the rest of the logic in each branch. I kept it separate like in the original snippet.
aokolnychyi
force-pushed
the
date-time-utils
branch
from
410f187 to
e8b7493
Compare
|
Thanks for reviewing, @nastra @szehon-ho @gaborkaszab! |
4 participants
While working on time-related functions in the Spark function catalog, I wanted to use
Transform. However, calling transforms on primitive values would require unnecessary boxing. That's why I decided to move relevant conversions toDateTimeUtiland leverage the utility directly. It is in line with what we did for bucketing in PR #5513.