You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by mgaido91 <gi...@git.apache.org> on 2018/04/30 19:17:22 UTC
[GitHub] spark pull request #21196: [SPARK-24123][SQL] Fix precision issues in months...
GitHub user mgaido91 opened a pull request:
https://github.com/apache/spark/pull/21196
[SPARK-24123][SQL] Fix precision issues in monthsBetween with more than 8 digits
## What changes were proposed in this pull request?
SPARK-23902 introduced the ability to retrieve more than 8 digits in `monthsBetween`. Unfortunately, current implementation can cause precision loss in such a case. This was causing also a flaky UT.
This PR mirrors Hive's implementation in order to avoid precision loss also when more than 8 digits are returned.
## How was this patch tested?
running 10000000 times the flaky UT
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mgaido91/spark SPARK-24123
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21196.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #21196
----
commit c398b0455e53355933dc96198bc9f643c3e0c105
Author: Marco Gaido <ma...@...>
Date: 2018-04-30T19:06:18Z
[SPARK-24123][SQL] Fix precision issues in monthsBetween with more than 8 digits
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/21196
The failure is irrelevant to this PR.
```
org.apache.spark.sql.hive.HiveExternalCatalogVersionsSuite.(It is not a test it is a sbt.testing.SuiteSelector)
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/2765/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/21196
Retest this please.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21196: [SPARK-24123][SQL] Fix precision issues in months...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21196
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21196
**[Test build #89997 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89997/testReport)** for PR 21196 at commit [`4d1775c`](https://github.com/apache/spark/commit/4d1775c2dc40cbbed88042803d21804a6f6ab1dd).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/90071/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/21196
Retest this please.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21196: [SPARK-24123][SQL] Fix precision issues in months...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/21196#discussion_r185104899
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala ---
@@ -45,6 +45,7 @@ object DateTimeUtils {
// it's 2440587.5, rounding up to compatible with Hive
final val JULIAN_DAY_OF_EPOCH = 2440588
final val SECONDS_PER_DAY = 60 * 60 * 24L
+ final val SECONDS_PER_MONTH = 60 * 60 * 24 * 31D
--- End diff --
Thank you for working on this, @mgaido91 !
BTW, is the naming, `SECONDS_PER_MONTH`, correct for February?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21196: [SPARK-24123][SQL] Fix precision issues in months...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21196#discussion_r185115238
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala ---
@@ -45,6 +45,7 @@ object DateTimeUtils {
// it's 2440587.5, rounding up to compatible with Hive
final val JULIAN_DAY_OF_EPOCH = 2440588
final val SECONDS_PER_DAY = 60 * 60 * 24L
+ final val SECONDS_PER_MONTH = 60 * 60 * 24 * 31D
--- End diff --
I see now your point, yes, I will inline the value as Hive does with a comment then, thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21196
The R test failure is not related. Thanks! Merged to master
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/89984/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/21196
Thank you, @gatorsmile and @mgaido91 !
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21196: [SPARK-24123][SQL] Fix precision issues in months...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/21196#discussion_r185634058
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala ---
@@ -888,14 +888,19 @@ object DateTimeUtils {
val months1 = year1 * 12 + monthInYear1
val months2 = year2 * 12 + monthInYear2
+ val monthDiff = (months1 - months2).toDouble
+
if (dayInMonth1 == dayInMonth2 || ((daysToMonthEnd1 == 0) && (daysToMonthEnd2 == 0))) {
- return (months1 - months2).toDouble
+ return monthDiff
}
- // milliseconds is enough for 8 digits precision on the right side
- val timeInDay1 = millis1 - daysToMillis(date1, timeZone)
- val timeInDay2 = millis2 - daysToMillis(date2, timeZone)
- val timesBetween = (timeInDay1 - timeInDay2).toDouble / MILLIS_PER_DAY
- val diff = (months1 - months2).toDouble + (dayInMonth1 - dayInMonth2 + timesBetween) / 31.0
+ // using milliseconds can cause precision loss with more than 8 digits
+ // we follow Hive's implementation which uses seconds
--- End diff --
I checked how Hive works. It works as this comment says.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21196: [SPARK-24123][SQL] Fix precision issues in months...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/21196#discussion_r185114200
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala ---
@@ -45,6 +45,7 @@ object DateTimeUtils {
// it's 2440587.5, rounding up to compatible with Hive
final val JULIAN_DAY_OF_EPOCH = 2440588
final val SECONDS_PER_DAY = 60 * 60 * 24L
+ final val SECONDS_PER_MONTH = 60 * 60 * 24 * 31D
--- End diff --
Nit, but both Spark and Hive avoid declaring it as a variable (maybe due to the same reason) and mentioned that in the comments only.
- https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFMonthsBetween.java#L140
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/2825/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/21196
SparkR failure is irrelevant to this PR and occurs in several other PRs. We had better wait for fixing SparkR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21196: [SPARK-24123][SQL] Fix precision issues in months...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21196#discussion_r185106149
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala ---
@@ -45,6 +45,7 @@ object DateTimeUtils {
// it's 2440587.5, rounding up to compatible with Hive
final val JULIAN_DAY_OF_EPOCH = 2440588
final val SECONDS_PER_DAY = 60 * 60 * 24L
+ final val SECONDS_PER_MONTH = 60 * 60 * 24 * 31D
--- End diff --
hi @dongjoon-hyun! thanks for the comment. Of course it is not, strictly speaking, but this is how both Spark before the PR (notice the 31.0) and Hive (which Spark draws his inspiration from for this function) work. They consider every month as having 31 days, even though this is not true of course.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21196
**[Test build #89984 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89984/testReport)** for PR 21196 at commit [`4d1775c`](https://github.com/apache/spark/commit/4d1775c2dc40cbbed88042803d21804a6f6ab1dd).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21196: [SPARK-24123][SQL] Fix precision issues in months...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/21196#discussion_r185634570
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala ---
@@ -888,14 +888,19 @@ object DateTimeUtils {
val months1 = year1 * 12 + monthInYear1
val months2 = year2 * 12 + monthInYear2
+ val monthDiff = (months1 - months2).toDouble
+
if (dayInMonth1 == dayInMonth2 || ((daysToMonthEnd1 == 0) && (daysToMonthEnd2 == 0))) {
- return (months1 - months2).toDouble
+ return monthDiff
}
- // milliseconds is enough for 8 digits precision on the right side
- val timeInDay1 = millis1 - daysToMillis(date1, timeZone)
- val timeInDay2 = millis2 - daysToMillis(date2, timeZone)
- val timesBetween = (timeInDay1 - timeInDay2).toDouble / MILLIS_PER_DAY
- val diff = (months1 - months2).toDouble + (dayInMonth1 - dayInMonth2 + timesBetween) / 31.0
+ // using milliseconds can cause precision loss with more than 8 digits
+ // we follow Hive's implementation which uses seconds
--- End diff --
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/89997/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/2756/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21196
**[Test build #90071 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90071/testReport)** for PR 21196 at commit [`4d1775c`](https://github.com/apache/spark/commit/4d1775c2dc40cbbed88042803d21804a6f6ab1dd).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21196
**[Test build #89984 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89984/testReport)** for PR 21196 at commit [`4d1775c`](https://github.com/apache/spark/commit/4d1775c2dc40cbbed88042803d21804a6f6ab1dd).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21196
**[Test build #90071 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90071/testReport)** for PR 21196 at commit [`4d1775c`](https://github.com/apache/spark/commit/4d1775c2dc40cbbed88042803d21804a6f6ab1dd).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21196
cc @ueshin
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21196
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/21196
Retest this please.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21196: [SPARK-24123][SQL] Fix precision issues in monthsBetween...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21196
**[Test build #89997 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89997/testReport)** for PR 21196 at commit [`4d1775c`](https://github.com/apache/spark/commit/4d1775c2dc40cbbed88042803d21804a6f6ab1dd).
* This patch **fails SparkR unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org