[SPARK-20763][SQL]The function of `month` and `day` return the value which is not we expected. #17997

10110346 · 2017-05-16T09:20:22Z

What changes were proposed in this pull request?

spark-sql>select month("1582-09-28");
spark-sql>10
For this case, the expected result is 9, but it is 10.

spark-sql>select day("1582-04-18");
spark-sql>28
For this case, the expected result is 18, but it is 28.

when the date before "1582-10-04", the function of month and day return the value which is not we expected.

How was this patch tested?

unit tests

srowen · 2017-05-16T10:52:24Z

Point taken, but do other DBs support dates before 1970? it does highly depend on historical calendars. The right solution isn't a hack in one place here but using Calendar properly, if anything.

10110346 · 2017-05-16T10:59:07Z

@srowen I have tested in mysql, it can support dates before 1970.
mysql> select month("1582-09-28");
+---------------------+
| month("1582-09-28") |
+---------------------+
| 9 |
+---------------------+

mysql> select day("1582-04-18");
+-------------------+
| day("1582-04-18") |
+-------------------+
| 18 |
+-------------------+

rxin · 2017-05-16T23:03:44Z

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

-    (year, dayInYear)
+  private[this] def getYearAndDayInYear(daysSince1970: SQLDate): (Int, Int, Int) = {
+    val date = new Date(daysToMillis(daysSince1970))
+    val YMD = date.toString.trim.split("-")


this would cause massive performance regression.

yes,this cause performance worse than before

10110346 · 2017-05-17T00:37:40Z

@srowen @rxin So, by contrast, maybe make a hack in one place is a better solution

ueshin · 2017-05-17T05:27:24Z

I guess this would also affect date-related functions such as dayofyear, year, quater and weekofyear, etc.
Could you add tests for them?

10110346 · 2017-05-17T05:35:49Z

@ueshin Yes, I will do it ,thanks

ueshin · 2017-05-17T07:19:24Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

@@ -76,6 +76,9 @@ class DateExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
      }
    }
    checkEvaluation(DayOfYear(Literal.create(null, DateType)), null)
+
+    checkEvaluation(DayOfYear(Literal(new Date(sdf.parse("1582-04-28 13:10:15").getTime))), 118)
+    checkEvaluation(DayOfYear(Literal(new Date(sdf.parse("1582-10-4 13:10:15").getTime))), 277)


I believe the dates we need to test here should be the boundaries like 1582-10-04 and 1582-10-15.

Btw, let's add 0 for 1-digit date, e.g. 1582-10-04 instead of 1582-10-4.

ueshin · 2017-05-17T07:47:03Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

@@ -96,6 +99,8 @@ class DateExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
        }
      }
    }
+    checkEvaluation(Year(Literal(new Date(sdf.parse("1582-01-01 13:10:15").getTime))), 1582)
+    checkEvaluation(Year(Literal(new Date(sdf.parse("1582-12-31 13:10:15").getTime))), 1582)


Here should be 1581-12-31 and 1582-01-01, for example, and the rest should be in a similar way.

ueshin · 2017-05-17T09:04:49Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

@@ -116,6 +121,9 @@ class DateExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
        }
      }
    }
+
+    checkEvaluation(Quarter(Literal(new Date(sdf.parse("1582-03-28 13:10:15").getTime))), 1)
+    checkEvaluation(Quarter(Literal(new Date(sdf.parse("1582-09-30 13:10:15").getTime))), 3)


How about using 1582-09-30 and 1582-10-01 for this?

ueshin · 2017-05-17T09:10:21Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

@@ -76,6 +76,9 @@ class DateExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
      }
    }
    checkEvaluation(DayOfYear(Literal.create(null, DateType)), null)
+
+    checkEvaluation(DayOfYear(Literal(new Date(sdf.parse("1582-10-15 13:10:15").getTime))), 288)


I'm not sure whether this value is correct or not. should be 278?

I'm not sure too, maybe 278 is better

cc @cloud-fan @gatorsmile
Do you have any ideas?

can we check with other databases?

@cloud-fan In mysql ,the rusult is :
mysql> select dayofyear("1982-10-04");
+-------------------------+
| dayofyear("1982-10-04") |
+-------------------------+
| 277 |
+-------------------------+
1 row in set (0.00 sec)

mysql> select dayofyear("1982-10-15");
+--------------------------+
| dayofyear("1982-10-15") |
+--------------------------+
| 288 |
+--------------------------+

why 278 is better?

@cloud-fan Because in history,the period(5.10.1582 ~ 14.10.1582) is not exist.
But maybe 288 is more human readable.

let's follow mysql

OK, thanks @cloud-fan

kiszk · 2017-05-17T09:19:30Z

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

@@ -603,7 +603,13 @@ object DateTimeUtils {
   */
  private[this] def getYearAndDayInYear(daysSince1970: SQLDate): (Int, Int) = {
    // add the difference (in days) between 1.1.1970 and the artificial year 0 (-17999)
-    val daysNormalized = daysSince1970 + toYearZero
+    var  daysSince1970Tmp = daysSince1970
+    // In history,the period(5.10.1582 ~ 14.10.1582) is not exist


nit: this representation may be confusing for all of the people in the world. Can we use 1582-10-5 or like Oct. 5, 1582?

It's only about comment, and I think 1582-10-5 or Oct. 5, 1582 is more human readable.

OK, I will do ,thanks @kiszk @cloud-fan

In history,the period(5.10.1582 ~ 14.10.1582) is not exist
->
Since Julian calendar was replaced with the Gregorian calendar, the 10 days after Oct. 4 were skipped.

ueshin · 2017-05-18T06:53:07Z

ok to test

gatorsmile · 2017-05-18T07:36:57Z

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

@@ -603,7 +603,13 @@ object DateTimeUtils {
   */
  private[this] def getYearAndDayInYear(daysSince1970: SQLDate): (Int, Int) = {
    // add the difference (in days) between 1.1.1970 and the artificial year 0 (-17999)
-    val daysNormalized = daysSince1970 + toYearZero
+    var  daysSince1970Tmp = daysSince1970
+    // In history,the period(1582-10-05 ~ 1582-10-14) is not exist


In history,the period(5.10.1582 ~ 14.10.1582) is not exist
->
Since Julian calendar was replaced with the Gregorian calendar, the 10 days after Oct. 4 were skipped.

@gatorsmile ok,thanks

gatorsmile · 2017-05-18T18:23:26Z

The current impl is still missing the special handling of that miracle period. See the outputs of Oracle:

Result Set 34
TO_CHAR(TO_DATE('1582-10-04','YYYY-MM-DD'),'DDD')
277

Result Set 35
TO_CHAR(TO_DATE('1582-10-05','YYYY-MM-DD'),'DDD')
288

Result Set 36
TO_CHAR(TO_DATE('1582-10-11','YYYY-MM-DD'),'DDD')
288

Result Set 37
TO_CHAR(TO_DATE('1582-10-12','YYYY-MM-DD'),'DDD')
288

Result Set 38
TO_CHAR(TO_DATE('1582-10-13','YYYY-MM-DD'),'DDD')
288

Result Set 39
TO_CHAR(TO_DATE('1582-10-14','YYYY-MM-DD'),'DDD')
288

Result Set 40
TO_CHAR(TO_DATE('1582-10-15','YYYY-MM-DD'),'DDD')
288

Result Set 41
TO_CHAR(TO_DATE('1582-10-16','YYYY-MM-DD'),'DDD')
289

10110346 · 2017-05-19T06:54:20Z

@gatorsmile I have tested like this:
val dav = Date.valueOf("1582-10-04"); val date = new Date(dav.getTime); println(date.toString)
the output is :1582-10-04
val dav = Date.valueOf("1582-10-05"); val date = new Date(dav.getTime); println(date.toString)
the output is :1582-10-15
val dav = Date.valueOf("1582-10-14"); val date = new Date(dav.getTime); println(date.toString)
the output is :1582-10-24
val dav = Date.valueOf("1582-10-15"); val date = new Date(dav.getTime); println(date.toString)
the output is :1582-10-15

Also,i tested in mysql:
mysql> select dayofyear('1582-10-4');
+------------------------+
| dayofyear('1582-10-4') |
+------------------------+
| 277 |
+------------------------+
1 row in set (0.00 sec)

mysql> select dayofyear('1582-10-5');
+------------------------+
| dayofyear('1582-10-5') |
+------------------------+
| 278 |
+------------------------+
1 row in set (0.00 sec)

mysql> select dayofyear('1582-10-6');
+------------------------+
| dayofyear('1582-10-6') |
+------------------------+
| 279 |
+------------------------+
1 row in set (0.00 sec)

mysql> select dayofyear('1582-10-14');
+-------------------------+
| dayofyear('1582-10-14') |
+-------------------------+
| 287 |
+-------------------------+
1 row in set (0.00 sec)

mysql> select dayofyear('1582-10-15');
+-------------------------+
| dayofyear('1582-10-15') |
+-------------------------+
| 288 |
+-------------------------+

gatorsmile · 2017-05-19T17:24:24Z

After checking DB2, it behaves the same as MySQL.

LGTM

… which is not we expected. ## What changes were proposed in this pull request? spark-sql>select month("1582-09-28"); spark-sql>10 For this case, the expected result is 9, but it is 10. spark-sql>select day("1582-04-18"); spark-sql>28 For this case, the expected result is 18, but it is 28. when the date before "1582-10-04", the function of `month` and `day` return the value which is not we expected. ## How was this patch tested? unit tests Author: liuxian <liu.xian3@zte.com.cn> Closes #17997 from 10110346/wip_lx_0516. (cherry picked from commit ea3b1e3) Signed-off-by: Xiao Li <gatorsmile@gmail.com>

gatorsmile · 2017-05-19T17:26:02Z

Thanks! Merging to master/2.2.

gatorsmile · 2017-05-19T17:26:18Z

Could you submit a backport PR to 2.1? Thanks!

10110346 · 2017-05-19T21:38:23Z

OK，l Will do it, thanks. @gatorsmile

…turn the value which is not we expected. What changes were proposed in this pull request? This PR is to backport #17997 to Spark 2.1 when the date before "1582-10-04", the function of month and day return the value which is not we expected. How was this patch tested? unit tests Author: liuxian <liu.xian3@zte.com.cn> Closes #18054 from 10110346/wip-lx-0522.

… which is not we expected. ## What changes were proposed in this pull request? spark-sql>select month("1582-09-28"); spark-sql>10 For this case, the expected result is 9, but it is 10. spark-sql>select day("1582-04-18"); spark-sql>28 For this case, the expected result is 18, but it is 28. when the date before "1582-10-04", the function of `month` and `day` return the value which is not we expected. ## How was this patch tested? unit tests Author: liuxian <liu.xian3@zte.com.cn> Closes apache#17997 from 10110346/wip_lx_0516.

The function of month and day return an error value

7d6385f

10110346 changed the title ~~[SPARK-20763][SQL]The function of month and day return an error value~~ [SPARK-20763][SQL]The function of month and day return the value which is not we expected. May 16, 2017

10110346 force-pushed the wip_lx_0516 branch from e096d47 to d188c11 Compare May 16, 2017 12:35

rxin reviewed May 16, 2017

View reviewed changes

10110346 force-pushed the wip_lx_0516 branch 2 times, most recently from 3f2d8d6 to a8d6e04 Compare May 17, 2017 02:21

10110346 force-pushed the wip_lx_0516 branch from a8d6e04 to 2e87481 Compare May 17, 2017 06:29

ueshin reviewed May 17, 2017

View reviewed changes

10110346 force-pushed the wip_lx_0516 branch from 2e87481 to c50755f Compare May 17, 2017 08:15

ueshin reviewed May 17, 2017

View reviewed changes

kiszk reviewed May 17, 2017

View reviewed changes

10110346 force-pushed the wip_lx_0516 branch 2 times, most recently from 205d3d0 to 8c99316 Compare May 18, 2017 05:29

gatorsmile reviewed May 18, 2017

View reviewed changes

10110346 force-pushed the wip_lx_0516 branch from 8c99316 to 3767baf Compare May 18, 2017 07:49

Fix:add unit test cases

3430e94

10110346 force-pushed the wip_lx_0516 branch from 3767baf to 3430e94 Compare May 18, 2017 11:42

asfgit closed this in ea3b1e3 May 19, 2017

10110346 deleted the wip_lx_0516 branch May 22, 2017 00:56

10110346 restored the wip_lx_0516 branch May 22, 2017 00:57

This was referenced May 22, 2017

[SPARK-20763][SQL][Backport-2.1] The function of month and day return the value which is not we expected. #18053

Closed

[SPARK-20763][SQL][Backport-2.1] The function of month and day return the value which is not we expected. #18054

Closed

[SPARK-20763][SQL]The function of month and day return the value which is not we expected. #17997

[SPARK-20763][SQL]The function of month and day return the value which is not we expected. #17997

Uh oh!

Conversation

10110346 commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen commented May 16, 2017

Uh oh!

10110346 commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

10110346 commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ueshin commented May 17, 2017

Uh oh!

10110346 commented May 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

10110346 May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

10110346 May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ueshin commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented May 18, 2017

Uh oh!

10110346 commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gatorsmile commented May 19, 2017

Uh oh!

gatorsmile commented May 19, 2017

Uh oh!

gatorsmile commented May 19, 2017

Uh oh!

10110346 commented May 19, 2017

Uh oh!

Uh oh!

[SPARK-20763][SQL]The function of `month` and `day` return the value which is not we expected. #17997

[SPARK-20763][SQL]The function of `month` and `day` return the value which is not we expected. #17997

10110346 commented May 16, 2017 •

edited

Loading

10110346 commented May 17, 2017 •

edited

Loading

10110346 May 18, 2017 •

edited

Loading

10110346 May 18, 2017 •

edited

Loading

10110346 commented May 19, 2017 •

edited

Loading