[SPARK-31579][SQL] replaced floorDiv to Div #29008
Conversation
There was a problem hiding this comment.
Yes, the comma needs to become a slash.
I think this change would then be OK as the sign of these two args are always the same.
However, does it make much difference? it's going to be a few extra math ops. It might be useful to just keep this consistent
There was a problem hiding this comment.
Oops Sorry, silly me.
And yes @srowen, this does make things faster. I can do some benchmarking if you would like me to...
There was a problem hiding this comment.
I guess I'd be surprised if the operations were meaningfully faster, given how much else is happening. I'm sure a microbenchmark of a trillion calls would show a difference, but that's not quite the use case.
Nevertheless are there other instances where both args are definitely nonnegative?
There was a problem hiding this comment.
Here, the question is not about performance but about our understanding of the logic.
If we set only date fields:
.setDate(localDate.getYear, localDate.getMonthValue - 1, localDate.getDayOfMonth)the time part MUST be zero. If it is not, something is wrong going on.
I would add the assert before the division:
assert(utcCal.getTimeInMillis % MILLIS_PER_DAY == 0)
| .setDate(localDate.getYear, localDate.getMonthValue - 1, localDate.getDayOfMonth) | ||
| .build() | ||
| Math.toIntExact(Math.floorDiv(utcCal.getTimeInMillis, MILLIS_PER_DAY)) | ||
| Math.toIntExact(utcCal.getTimeInMillis/MILLIS_PER_DAY) |
There was a problem hiding this comment.
| Math.toIntExact(utcCal.getTimeInMillis/MILLIS_PER_DAY) | |
| Math.toIntExact(utcCal.getTimeInMillis / MILLIS_PER_DAY) |
|
@Sudhar287 Please, clean up PR's description, and the title (it is so generic). |
|
PTAL @MaxGekk :) |
|
Jenkins test this please |
|
Test build #125894 has started for PR 29008 at commit |
|
Hmm, it says no failures here. What am I suppose to do? |
|
Jenkins retest this please |
|
Test build #125977 has finished for PR 29008 at commit
|
|
Jenkins retest this please |
|
Test build #126066 has finished for PR 29008 at commit
|
|
Merged to master |
|
Awesome. Thanks! |
4 participants
What changes were proposed in this pull request?
Replaced floorDiv to just / in
localRebaseGregorianToJulianDays()inspark/sql/catalyst/util/RebaseDateTime.scalaWhy are the changes needed?
Easier to understand the logic/code and a little more efficiency.
Does this PR introduce any user-facing change?
NO
How was this patch tested?
Proof of concept here. The operation
utcCal.getTimeInMillis / MILLIS_PER_DAYresults in an interger value already.