You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/06/21 12:58:09 UTC

[GitHub] [spark] MaxGekk opened a new pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

MaxGekk opened a new pull request #28886:
URL: https://github.com/apache/spark/pull/28886


   ### What changes were proposed in this pull request?
   Replace Decimal by Int op in the `MakeInterval` & `MakeTimestamp` expression. For instance, `(secs * Decimal(MICROS_PER_SECOND)).toLong` can be replaced by the unscaled long because the former one already contains microseconds.
   
   ### Why are the changes needed?
   To improve performance.
   
   Before:
   ```
   make_timestamp():                         Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
   ------------------------------------------------------------------------------------------------------------------------
   ...
   make_timestamp(2019, 1, 2, 3, 4, 50.123456)             94             99           4         10.7          93.8      38.8X
   ```
   
   After:
   ```
   make_timestamp(2019, 1, 2, 3, 4, 50.123456)             76             92          15         13.1          76.5      48.1X
   ```
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   ### How was this patch tested?
   - By existing test suites `IntervalExpressionsSuite`, `DateExpressionsSuite` and etc.
   - Re-generate results of `MakeDateTimeBenchmark` in the environment:
   
   | Item | Description |
   | ---- | ----|
   | Region | us-west-2 (Oregon) |
   | Instance | r3.xlarge |
   | AMI | ubuntu/images/hvm-ssd/ubuntu-bionic-18.04-amd64-server-20190722.1 (ami-06f2f779464715dc5) |
   | Java | OpenJDK 64-Bit Server VM 1.8.0_252 and OpenJDK 64-Bit Server VM 11.0.7+10 |
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r443997431



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##########
@@ -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")

Review comment:
       shall we check the precision as well?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28886:
URL: https://github.com/apache/spark/pull/28886#issuecomment-647158733


   **[Test build #124337 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124337/testReport)** for PR 28886 at commit [`b9e2ee0`](https://github.com/apache/spark/commit/b9e2ee097bdbe6c6ff1d5680025ade7cc9fca9e7).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] MaxGekk commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r444023848



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##########
@@ -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")

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] MaxGekk commented on pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #28886:
URL: https://github.com/apache/spark/pull/28886#issuecomment-647166178


   @dongjoon-hyun While working on https://github.com/apache/spark/pull/28873, I had realised that some Decimal ops are not needed actually, and they can be replaced by regular int ops. May I ask you to take a look at this.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28886:
URL: https://github.com/apache/spark/pull/28886#issuecomment-647158959






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] MaxGekk commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r444154153



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##########
@@ -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")

Review comment:
       Not anymore, please, take a look at https://github.com/apache/spark/pull/28873 and https://issues.apache.org/jira/browse/SPARK-32021




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] MaxGekk commented on pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #28886:
URL: https://github.com/apache/spark/pull/28886#issuecomment-647669116


   @cloud-fan @juliuszsompolski @HyukjinKwon Could you review this small perf improvement.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28886:
URL: https://github.com/apache/spark/pull/28886#issuecomment-647125736






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] MaxGekk commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r444081799



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##########
@@ -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")

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] YMajid commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
YMajid commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r443267050



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala
##########
@@ -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,

Review comment:
       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!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan closed pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #28886:
URL: https://github.com/apache/spark/pull/28886


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r444079619



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##########
@@ -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")

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] MaxGekk commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r444154153



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##########
@@ -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")

Review comment:
       Not anymore, please, take a look at https://github.com/apache/spark/pull/28873




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28886:
URL: https://github.com/apache/spark/pull/28886#issuecomment-647125617


   **[Test build #124337 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124337/testReport)** for PR 28886 at commit [`b9e2ee0`](https://github.com/apache/spark/commit/b9e2ee097bdbe6c6ff1d5680025ade7cc9fca9e7).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] YMajid commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
YMajid commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r443553133



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala
##########
@@ -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,

Review comment:
       Awesome, thank you so much!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r444041607



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##########
@@ -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")

Review comment:
       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`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28886:
URL: https://github.com/apache/spark/pull/28886#issuecomment-647125617


   **[Test build #124337 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124337/testReport)** for PR 28886 at commit [`b9e2ee0`](https://github.com/apache/spark/commit/b9e2ee097bdbe6c6ff1d5680025ade7cc9fca9e7).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] MaxGekk commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r444003827



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##########
@@ -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")

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r444151837



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##########
@@ -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")

Review comment:
       for sanity check? The `precision` must be `<= 8` here, right?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #28886:
URL: https://github.com/apache/spark/pull/28886#issuecomment-648093682


   thanks, merging to master!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] MaxGekk commented on a change in pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28886:
URL: https://github.com/apache/spark/pull/28886#discussion_r443360334



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala
##########
@@ -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,

Review comment:
       > 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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28886:
URL: https://github.com/apache/spark/pull/28886#issuecomment-647125736






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #28886: [SPARK-32043][SQL] Replace Decimal by Int op in `make_interval` and `make_timestamp`

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28886:
URL: https://github.com/apache/spark/pull/28886#issuecomment-647158959






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org