Skip to content

[SPARK-32043][SQL] Replace Decimal by Int op in make_interval and make_timestamp #28886

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MaxGekk wants to merge 4 commits into from
Closed

[SPARK-32043][SQL] Replace Decimal by Int op in make_interval and make_timestamp #28886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
713bb7e
Avoid decimal op in makeInterval
MaxGekk Jun 21, 2020
f733864
Avoid decimal op in toMicros
MaxGekk Jun 21, 2020
ac7939a
Re-gen MakeDateTimeBenchmark-jdk11-results.txt
MaxGekk Jun 21, 2020
b9e2ee0
Re-gen MakeDateTimeBenchmark-results.txt
MaxGekk Jun 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions ...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1994,13 +1994,17 @@ case class MakeTimestamp(
day: Int,
hour: Int,
min: Int,
secAndNanos: Decimal,
secAndMicros: Decimal,
zoneId: ZoneId): Any = {
try {
val secFloor = secAndNanos.floor
val nanosPerSec = Decimal(NANOS_PER_SECOND, 10, 0)
val nanos = ((secAndNanos - secFloor) * nanosPerSec).toInt
val seconds = secFloor.toInt
assert(secAndMicros.scale == 6,
Copy link

@ymajid ymajid Jun 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt that this is the right place to ask, but I tried to Google this but didn't find anything. I was just wondering what does assert(secAndMicros.scale == 6 do exactly? I am assuming that it changes the Decimal's scale to 6, but what does that exactly help with? And is the added tail just a bunch of zeros? For instance does 3.14 become 3.140000?
Thanks!

Copy link
Member Author

@MaxGekk MaxGekk Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming that it changes the Decimal's scale to 6

This assert doesn't change the scale, it just reads it.

And is the added tail just a bunch of zeros? For instance does 3.14 become 3.140000?

It shifts the decimal point. For example, if you have 3.14 with precision 6 and scale 3 that means 003.140. If you set

  • scale to 0, it becomes 3140
  • scale to 4 -> 0.314

Copy link

@ymajid ymajid Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you so much!

s"Seconds fraction must have 6 digits for microseconds but got ${secAndMicros.scale}")
val unscaledSecFrac = secAndMicros.toUnscaledLong
assert(secAndMicros.precision <= 8,
s"Seconds and fraction cannot have more than 8 digits but got ${secAndMicros.precision}")
val totalMicros = unscaledSecFrac.toInt // 8 digits cannot overflow Int
val seconds = Math.floorDiv(totalMicros, MICROS_PER_SECOND.toInt)
val nanos = Math.floorMod(totalMicros, MICROS_PER_SECOND.toInt) * NANOS_PER_MICROS.toInt
val ldt = if (seconds == 60) {
if (nanos == 0) {
// This case of sec = 60 and nanos = 0 is supported for compatibility with PostgreSQL
Expand Down
3 changes: 2 additions & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,8 @@ object IntervalUtils {
secs: Decimal): CalendarInterval = {
val totalMonths = Math.addExact(months, Math.multiplyExact(years, MONTHS_PER_YEAR))
val totalDays = Math.addExact(days, Math.multiplyExact(weeks, DAYS_PER_WEEK))
var micros = (secs * Decimal(MICROS_PER_SECOND)).toLong
assert(secs.scale == 6, "Seconds fractional must have 6 digits for microseconds")
Copy link
Contributor

@cloud-fan cloud-fan Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we check the precision as well?

Copy link
Member Author

@MaxGekk MaxGekk Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can check it but precision value is not important for this code...or I am wrong?

Copy link
Member Author

@MaxGekk MaxGekk Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, Decimal guarantees that precision >= scale (@Ngone51 correct?). That's enough for the code.

Copy link
Member

@Ngone51 Ngone51 Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, I think we should say DecimalType guarantees precision >= scale. But since any Decimal is directly or indirectly bounded with a DecimalType. Therefore, Decimal also guarantees precision >= scale.

Copy link
Contributor

@cloud-fan cloud-fan Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to be consistent with https://github.com/apache/spark/pull/28886/files#diff-b83497f7bc11578a0b63a814a2a30f48R2003

Copy link
Member Author

@MaxGekk MaxGekk Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added assert there to guarantee that Int will not overflow. What would you like to assert here?

Copy link
Contributor

@cloud-fan cloud-fan Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sanity check? The precision must be <= 8 here, right?

Copy link
Member Author

@MaxGekk MaxGekk Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not anymore, please, take a look at #28873 and https://issues.apache.org/jira/browse/SPARK-32021

var micros = secs.toUnscaledLong
micros = Math.addExact(micros, Math.multiplyExact(hours, MICROS_PER_HOUR))
micros = Math.addExact(micros, Math.multiplyExact(mins, MICROS_PER_MINUTE))

Expand Down
28 changes: 14 additions & 14 deletions sql/core/benchmarks/MakeDateTimeBenchmark-jdk11-results.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
OpenJDK 64-Bit Server VM 11.0.5+10-post-Ubuntu-0ubuntu1.118.04 on Linux 4.15.0-1044-aws
OpenJDK 64-Bit Server VM 11.0.7+10-post-Ubuntu-2ubuntu218.04 on Linux 4.15.0-1063-aws
Intel(R) Xeon(R) CPU E5-2670 v2 @ 2.50GHz
make_date(): Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
prepare make_date() 3204 3323 139 31.2 32.0 1.0X
make_date(2019, 9, 16) 2529 2604 126 39.5 25.3 1.3X
make_date(*, *, *) 5102 5113 10 19.6 51.0 0.6X
prepare make_date() 3170 3321 230 31.5 31.7 1.0X
make_date(2019, 9, 16) 2359 2566 343 42.4 23.6 1.3X
make_date(*, *, *) 4398 4455 53 22.7 44.0 0.7X

OpenJDK 64-Bit Server VM 11.0.5+10-post-Ubuntu-0ubuntu1.118.04 on Linux 4.15.0-1044-aws
OpenJDK 64-Bit Server VM 11.0.7+10-post-Ubuntu-2ubuntu218.04 on Linux 4.15.0-1063-aws
Intel(R) Xeon(R) CPU E5-2670 v2 @ 2.50GHz
make_timestamp(): Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
prepare make_timestamp() 3484 3513 28 0.3 3484.3 1.0X
make_timestamp(2019, 1, 2, 3, 4, 50.123456) 112 131 17 9.0 111.5 31.2X
make_timestamp(2019, 1, 2, 3, 4, 60.000000) 93 102 10 10.8 92.8 37.6X
make_timestamp(2019, 12, 31, 23, 59, 60.00) 85 88 4 11.8 84.8 41.1X
make_timestamp(*, *, *, 3, 4, 50.123456) 303 308 8 3.3 302.8 11.5X
make_timestamp(*, *, *, *, *, 0) 303 307 3 3.3 302.8 11.5X
make_timestamp(*, *, *, *, *, 60.0) 289 297 8 3.5 289.1 12.1X
make_timestamp(2019, 1, 2, *, *, *) 3576 3585 11 0.3 3576.4 1.0X
make_timestamp(*, *, *, *, *, *) 3610 3618 12 0.3 3610.0 1.0X
prepare make_timestamp() 3492 3536 70 0.3 3491.7 1.0X
make_timestamp(2019, 1, 2, 3, 4, 50.123456) 94 110 16 10.7 93.6 37.3X
make_timestamp(2019, 1, 2, 3, 4, 60.000000) 78 83 8 12.8 78.0 44.7X
make_timestamp(2019, 12, 31, 23, 59, 60.00) 66 72 6 15.1 66.2 52.7X
make_timestamp(*, *, *, 3, 4, 50.123456) 306 313 12 3.3 305.9 11.4X
make_timestamp(*, *, *, *, *, 0) 282 284 2 3.5 281.9 12.4X
make_timestamp(*, *, *, *, *, 60.0) 281 288 6 3.6 281.0 12.4X
make_timestamp(2019, 1, 2, *, *, *) 3637 3641 3 0.3 3636.7 1.0X
make_timestamp(*, *, *, *, *, *) 3635 3648 12 0.3 3634.9 1.0X

28 changes: 14 additions & 14 deletions sql/core/benchmarks/MakeDateTimeBenchmark-results.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
OpenJDK 64-Bit Server VM 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09 on Linux 4.15.0-1044-aws
OpenJDK 64-Bit Server VM 1.8.0_252-8u252-b09-1~18.04-b09 on Linux 4.15.0-1063-aws
Intel(R) Xeon(R) CPU E5-2670 v2 @ 2.50GHz
make_date(): Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
prepare make_date() 2920 3239 278 34.3 29.2 1.0X
make_date(2019, 9, 16) 2322 2371 61 43.1 23.2 1.3X
make_date(*, *, *) 4553 4560 6 22.0 45.5 0.6X
prepare make_date() 2951 3182 355 33.9 29.5 1.0X
make_date(2019, 9, 16) 2325 2415 101 43.0 23.2 1.3X
make_date(*, *, *) 4556 4573 17 21.9 45.6 0.6X

OpenJDK 64-Bit Server VM 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09 on Linux 4.15.0-1044-aws
OpenJDK 64-Bit Server VM 1.8.0_252-8u252-b09-1~18.04-b09 on Linux 4.15.0-1063-aws
Intel(R) Xeon(R) CPU E5-2670 v2 @ 2.50GHz
make_timestamp(): Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
prepare make_timestamp() 3636 3673 38 0.3 3635.7 1.0X
make_timestamp(2019, 1, 2, 3, 4, 50.123456) 94 99 4 10.7 93.8 38.8X
make_timestamp(2019, 1, 2, 3, 4, 60.000000) 68 80 13 14.6 68.3 53.2X
make_timestamp(2019, 12, 31, 23, 59, 60.00) 65 79 19 15.3 65.3 55.7X
make_timestamp(*, *, *, 3, 4, 50.123456) 271 280 14 3.7 270.7 13.4X
make_timestamp(*, *, *, *, *, 0) 255 263 11 3.9 255.5 14.2X
make_timestamp(*, *, *, *, *, 60.0) 254 258 4 3.9 254.2 14.3X
make_timestamp(2019, 1, 2, *, *, *) 3714 3722 8 0.3 3713.9 1.0X
make_timestamp(*, *, *, *, *, *) 3736 3741 6 0.3 3736.3 1.0X
prepare make_timestamp() 3677 3730 87 0.3 3677.0 1.0X
make_timestamp(2019, 1, 2, 3, 4, 50.123456) 76 92 15 13.1 76.5 48.1X
make_timestamp(2019, 1, 2, 3, 4, 60.000000) 70 72 3 14.4 69.7 52.8X
make_timestamp(2019, 12, 31, 23, 59, 60.00) 68 75 12 14.7 68.0 54.1X
make_timestamp(*, *, *, 3, 4, 50.123456) 266 277 12 3.8 265.8 13.8X
make_timestamp(*, *, *, *, *, 0) 258 286 46 3.9 258.3 14.2X
make_timestamp(*, *, *, *, *, 60.0) 261 269 11 3.8 260.7 14.1X
make_timestamp(2019, 1, 2, *, *, *) 3748 3762 22 0.3 3748.3 1.0X
make_timestamp(*, *, *, *, *, *) 3761 3762 2 0.3 3760.9 1.0X