[SPARK-35679][SQL] instantToMicros overflow #32839

dgd-contributor · 2021-06-09T07:48:08Z

Why are the changes needed?

With Long.minValue cast to an instant, secs will be floored in function microsToInstant and cause overflow when multiply with Micros_per_second

def microsToInstant(micros: Long): Instant = {
  val secs = Math.floorDiv(micros, MICROS_PER_SECOND)
  // Unfolded Math.floorMod(us, MICROS_PER_SECOND) to reuse the result of
  // the above calculation of `secs` via `floorDiv`.
  val mos = micros - secs * MICROS_PER_SECOND  <- it will overflow here
  Instant.ofEpochSecond(secs, mos * NANOS_PER_MICROS)
}

But the overflow is acceptable because it won't produce any change to the result

However, when convert the instant back to micro value, it will raise Overflow Error

def instantToMicros(instant: Instant): Long = {
  val us = Math.multiplyExact(instant.getEpochSecond, MICROS_PER_SECOND) <- It overflow here
  val result = Math.addExact(us, NANOSECONDS.toMicros(instant.getNano))
  result
}

Code to reproduce this error

instantToMicros(microToInstant(Long.MinValue))

Does this PR introduce any user-facing change?

No

How was this patch tested?

Test added

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

AmplabJenkins · 2021-06-09T08:27:03Z

Can one of the admins verify this patch?

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

gengliangwang · 2021-06-09T09:12:11Z

As I mentioned in #32814 (comment)

We can fix it but it is too corner. The fix makes performance worse, too.

I will leave this one to other committers. cc @MaxGekk @cloud-fan

Co-authored-by: Gengliang Wang <[email protected]>

dgd-contributor · 2021-06-09T09:16:23Z

As I mentioned in #32814 (comment)

We can fix it but it is too corner. The fix makes performance worse, too.

I will leave this one to other committers. cc @MaxGekk @cloud-fan

Thanks

cloud-fan

LGTM. I don't think perf is a big concern in this code path. And the overhead seems very small.

MaxGekk · 2021-06-09T10:19:21Z

@gengliangwang Thank for the ping. I will review this today.

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

MaxGekk · 2021-06-09T13:31:26Z

@dgd-contributor Regarding PR's description. Could you fix indentations in code examples according to the code style.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

MaxGekk · 2021-06-10T05:00:45Z

@dgd-contributor Does the issue exist in other versions: 3.0, 3.1?

MaxGekk · 2021-06-10T05:08:11Z

+1, LGTM. Merging to master, 3.1 and 3.0.
Thank you @dgd-contributor , and @cloud-fan for your review.

### Why are the changes needed? With Long.minValue cast to an instant, secs will be floored in function microsToInstant and cause overflow when multiply with Micros_per_second ``` def microsToInstant(micros: Long): Instant = { val secs = Math.floorDiv(micros, MICROS_PER_SECOND) // Unfolded Math.floorMod(us, MICROS_PER_SECOND) to reuse the result of // the above calculation of `secs` via `floorDiv`. val mos = micros - secs * MICROS_PER_SECOND <- it will overflow here Instant.ofEpochSecond(secs, mos * NANOS_PER_MICROS) } ``` But the overflow is acceptable because it won't produce any change to the result However, when convert the instant back to micro value, it will raise Overflow Error ``` def instantToMicros(instant: Instant): Long = { val us = Math.multiplyExact(instant.getEpochSecond, MICROS_PER_SECOND) <- It overflow here val result = Math.addExact(us, NANOSECONDS.toMicros(instant.getNano)) result } ``` Code to reproduce this error ``` instantToMicros(microToInstant(Long.MinValue)) ``` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Test added Closes #32839 from dgd-contributor/SPARK-35679_instantToMicro. Authored-by: dgd-contributor <[email protected]> Signed-off-by: Max Gekk <[email protected]> (cherry picked from commit aa3de40) Signed-off-by: Max Gekk <[email protected]>

With Long.minValue cast to an instant, secs will be floored in function microsToInstant and cause overflow when multiply with Micros_per_second ``` def microsToInstant(micros: Long): Instant = { val secs = Math.floorDiv(micros, MICROS_PER_SECOND) // Unfolded Math.floorMod(us, MICROS_PER_SECOND) to reuse the result of // the above calculation of `secs` via `floorDiv`. val mos = micros - secs * MICROS_PER_SECOND <- it will overflow here Instant.ofEpochSecond(secs, mos * NANOS_PER_MICROS) } ``` But the overflow is acceptable because it won't produce any change to the result However, when convert the instant back to micro value, it will raise Overflow Error ``` def instantToMicros(instant: Instant): Long = { val us = Math.multiplyExact(instant.getEpochSecond, MICROS_PER_SECOND) <- It overflow here val result = Math.addExact(us, NANOSECONDS.toMicros(instant.getNano)) result } ``` Code to reproduce this error ``` instantToMicros(microToInstant(Long.MinValue)) ``` No Test added Closes #32839 from dgd-contributor/SPARK-35679_instantToMicro. Authored-by: dgd-contributor <[email protected]> Signed-off-by: Max Gekk <[email protected]> (cherry picked from commit aa3de40) Signed-off-by: Max Gekk <[email protected]>

dgd-contributor · 2021-06-10T06:58:48Z

Thanks @MaxGekk @cloud-fan @gengliangwang for supporting

### Why are the changes needed? With Long.minValue cast to an instant, secs will be floored in function microsToInstant and cause overflow when multiply with Micros_per_second ``` def microsToInstant(micros: Long): Instant = { val secs = Math.floorDiv(micros, MICROS_PER_SECOND) // Unfolded Math.floorMod(us, MICROS_PER_SECOND) to reuse the result of // the above calculation of `secs` via `floorDiv`. val mos = micros - secs * MICROS_PER_SECOND <- it will overflow here Instant.ofEpochSecond(secs, mos * NANOS_PER_MICROS) } ``` But the overflow is acceptable because it won't produce any change to the result However, when convert the instant back to micro value, it will raise Overflow Error ``` def instantToMicros(instant: Instant): Long = { val us = Math.multiplyExact(instant.getEpochSecond, MICROS_PER_SECOND) <- It overflow here val result = Math.addExact(us, NANOSECONDS.toMicros(instant.getNano)) result } ``` Code to reproduce this error ``` instantToMicros(microToInstant(Long.MinValue)) ``` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Test added Closes apache#32839 from dgd-contributor/SPARK-35679_instantToMicro. Authored-by: dgd-contributor <[email protected]> Signed-off-by: Max Gekk <[email protected]> (cherry picked from commit aa3de40) Signed-off-by: Max Gekk <[email protected]>

dgd-contributor changed the title ~~[SPARK-35679][SQL] instantToMicros overflow~~ [SPARK-35679][SQL][WIP] instantToMicros overflow Jun 9, 2021

gengliangwang reviewed Jun 9, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated Show resolved Hide resolved

[SPARK-35679][SQL] instantToMicro overflow

249d299

dgd-contributor force-pushed the SPARK-35679_instantToMicro branch from 8977b16 to 249d299 Compare June 9, 2021 08:30

dgd-contributor requested a review from gengliangwang June 9, 2021 08:34

Update condition

1efb2d3

gengliangwang reviewed Jun 9, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated Show resolved Hide resolved

Update

0a6963e

Co-authored-by: Gengliang Wang <[email protected]>

cloud-fan approved these changes Jun 9, 2021

View reviewed changes

cloud-fan changed the title ~~[SPARK-35679][SQL][WIP] instantToMicros overflow~~ [SPARK-35679][SQL] instantToMicros overflow Jun 9, 2021

github-actions bot added the SQL label Jun 9, 2021

gengliangwang reviewed Jun 9, 2021

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Outdated Show resolved Hide resolved

MaxGekk requested changes Jun 9, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated Show resolved Hide resolved

Update approach from durationToMicro

377be9e

MaxGekk reviewed Jun 9, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated Show resolved Hide resolved

MaxGekk reviewed Jun 9, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Show resolved Hide resolved

MaxGekk reviewed Jun 9, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated Show resolved Hide resolved

MaxGekk reviewed Jun 9, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated Show resolved Hide resolved

dchvn added 2 commits June 10, 2021 08:22

constant value for Math.floorDiv

8f008a8

update

bcf67ac

cloud-fan reviewed Jun 10, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated Show resolved Hide resolved

Update constant

134c356

dgd-contributor requested a review from MaxGekk June 10, 2021 04:38

MaxGekk approved these changes Jun 10, 2021

View reviewed changes

MaxGekk closed this in aa3de40 Jun 10, 2021

[SPARK-35679][SQL] instantToMicros overflow #32839

[SPARK-35679][SQL] instantToMicros overflow #32839

Uh oh!

Conversation

dgd-contributor commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Uh oh!

AmplabJenkins commented Jun 9, 2021

Uh oh!

Uh oh!

gengliangwang commented Jun 9, 2021

Uh oh!

dgd-contributor commented Jun 9, 2021

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Jun 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaxGekk commented Jun 9, 2021

Uh oh!

Uh oh!

Uh oh!

MaxGekk commented Jun 10, 2021

Uh oh!

MaxGekk commented Jun 10, 2021

Uh oh!

dgd-contributor commented Jun 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dgd-contributor commented Jun 9, 2021 •

edited

Loading