[SPARK-28432][SQL] Add `make_date` function by MaxGekk · Pull Request #25210 · apache/spark

MaxGekk · 2019-07-20T12:19:35Z

What changes were proposed in this pull request?

New function make_date() takes 3 columns year, month and day, and makes new column of the DATE type. If values in the input columns are null or out of valid ranges, the function returns null. Valid ranges are:

year - [1, 9999]
month - [1, 12]
day - [1, 31]

Also constructed date must be valid otherwise make_date returns null.

The function is implemented similarly to make_date in PostgreSQL: https://www.postgresql.org/docs/11/functions-datetime.html to maintain feature parity with it.

Here is an example:

select make_date(2013, 7, 15);
2013-07-15

How was this patch tested?

Added new tests to DateExpressionsSuite.

SparkQA · 2019-07-20T16:27:23Z

Test build #107941 has finished for PR 25210 at commit fea2621.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-07-20T16:37:13Z

Thank you for working on this, @MaxGekk .
Since this is one function of SPARK-28432, I split SPARK-28432 into two JIRAs by cloning.
This one is for you and make_date. The new one is for make_timestamp.

cc @wangyum

dongjoon-hyun · 2019-07-20T16:38:09Z

The R failure seems to be CRAN check failure (which is irrelevant to this PR) again, but let's rerun this to confirm.

dongjoon-hyun · 2019-07-20T16:38:16Z

Retest this please.

MaxGekk · 2019-07-20T16:57:58Z

The R failure seems to be CRAN check failure (which is irrelevant to this PR) again ...

I think it is relevant. Need to describe parameters.

MaxGekk · 2019-07-20T17:54:16Z

@felixcheung @HyukjinKwon Could you take a look at the PR, especially on R related changes, please.

felixcheung

and add R test?

R/pkg/R/functions.R

SparkQA · 2019-07-20T20:43:33Z

Test build #107943 has finished for PR 25210 at commit fea2621.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2019-07-20T21:10:56Z

and add R test?

I will add a test but R tests crash with segmentation fault on my laptop with Mac OS.

SparkQA · 2019-07-20T21:52:37Z

Test build #107945 has finished for PR 25210 at commit a57754c.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-07-20T22:40:22Z

@MaxGekk . Got it. Is the following fixed at the latest commit?

* checking CRAN incoming feasibility ...Error in .check_package_CRAN_incoming(pkgdir) : 
  dims [product 24] do not match the length of object [0]

dongjoon-hyun · 2019-07-20T22:41:58Z

BTW, I'm wondering if we need to add this function for all. For hyperbolic math function, we only add them in SQL later because it's PostgreSQL compatibility effort.

SparkQA · 2019-07-21T01:28:30Z

Test build #107946 has finished for PR 25210 at commit 318cc18.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2019-07-21T02:02:22Z

BTW, I'm wondering if we need to add this function for all.

I see. I will remove the function from Scala, Python, R API.

SparkQA · 2019-07-21T06:00:52Z

Test build #107950 has finished for PR 25210 at commit a5ef08b.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-07-21T07:11:06Z

All test passed and only CRAN check fails. The failure is irrelevant to this PR.

* checking CRAN incoming feasibility ...Error in .check_package_CRAN_incoming(pkgdir) : 
  dims [product 24] do not match the length of object [0]
Execution halted

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

sql/core/src/test/resources/sql-tests/inputs/datetime.sql

MaxGekk · 2019-07-21T08:20:07Z

sql/core/src/test/resources/sql-tests/results/pgSQL/date.sql.out

+-- !query 48 schema
+struct<make_date(-44, 3, 15):date>
+-- !query 48 output
+0045-03-15


The year -44 is out of valid range according to SQL standard. We are getting 45 instead of -44 while converting to java.sql.Date. If you switch to Java 8 API for date/timestamps:

scala> spark.conf.set("spark.sql.datetime.java8API.enabled", true) scala> spark.sql("select make_date(-44, 3, 15)").collect res7: Array[org.apache.spark.sql.Row] = Array([-0044-03-15])

the returned instance of java.time.LocalDate seems reasonable.

You need to file a JIRA issue for this difference on make_date input range checking. Also, please add the JIRA id to date.sql file.

FYI, the following is PostgreSQL output.

https://github.com/postgres/postgres/blob/REL_12_BETA2/src/test/regress/expected/date.out

Actually, the reason for the difference here is how Spark converts internal type DateType to external one java.sql.Date (by default) or java.sql.LocalDate (when spark.sql.datetime.java8API.enabled is set to true). And how the external type converted to string. For example, to get the same format as PostgreSQL, need to provide the appropriate formatter.

For java.sql.Date:

spark.conf.set("spark.sql.datetime.java8API.enabled", false) val date = spark.sql("select make_date(-44, 3, 15)").first.getAs[java.sql.Date](0) val sdf = new java.text.SimpleDateFormat("MM-dd-yyyy G") scala> sdf.format(date) res18: String = 03-17-0045 BC

For Java8 java.time.LocalDate:

spark.conf.set("spark.sql.datetime.java8API.enabled", true) val formatter = DateTimeFormatter.ofPattern("MM-dd-yyyy G") val localDate = spark.sql("select make_date(-44, 3, 15)").first.getAs[LocalDate](0) localDate.format(formatter) res16: String = 03-15-0045 BC

The difference in days due to difference calendars (Julian in the first case, and Proleptic Gregorian in the second one).

I see Postgres formats the -44 year as 44 BC which is wrong according to ISO 8601. See https://en.wikipedia.org/wiki/Year_zero , for example:

The "basic" format for year 0 ... year 1 BC. ... hence -0001 = 2 BC.

I don't think we should implement Postgres bugs.

No, what I mean is we need a JIRA report and JIRA ID comment because we don't follow PostgreSQL like that.

Ideally, you can file a PostgreSQL bug and use that instead of SPARK JIRA.
In both ways, we should report the difference.

Here is the JIRA ticket: https://issues.apache.org/jira/browse/SPARK-28471

sql/core/src/test/resources/sql-tests/inputs/pgSQL/date.sql

SparkQA · 2019-07-21T12:04:42Z

Test build #107961 has finished for PR 25210 at commit 30e62c2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-21T17:56:54Z

Test build #107967 has finished for PR 25210 at commit 646456b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

gatorsmile · 2019-07-21T21:01:38Z

cc @ueshin

gatorsmile · 2019-07-21T21:02:52Z

@MaxGekk Could you also improve the PR description with the reasons why we need this new function?

HyukjinKwon

Looks good from my side.

ueshin

LGTM.

sql/core/src/test/resources/sql-tests/inputs/pgSQL/date.sql

SparkQA · 2019-07-22T12:14:23Z

Test build #107999 has finished for PR 25210 at commit 639c6b0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-22T13:12:31Z

Test build #108003 has finished for PR 25210 at commit 15c64d2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

+1, LGTM. Merged to master.
Thank you so much, @MaxGekk , @felixcheung , @HyukjinKwon , @gatorsmile , @ueshin !

MaxGekk added 9 commits July 19, 2019 22:28

Add make_date

65ce7ea

Add test for make_date

ccd7d59

Register the make_date function

1906e04

Tests for the make_date() function

60f5627

Fix usage in the description

d25e74d

Make make_date nullable

c82428c

Add a test to datetime.sql

2b257d0

Add make_date to PySpark

6b632dc

Add make_date to SparkR

fea2621

dongjoon-hyun changed the title ~~[SPARK-28432][SQL] New function - make_date~~ [SPARK-28432][SQL] Add make_date function Jul 20, 2019

dongjoon-hyun added the SQL label Jul 20, 2019

Fix docs for SparkR

a57754c

felixcheung reviewed Jul 20, 2019

View reviewed changes

R/pkg/R/functions.R Outdated Show resolved Hide resolved

R/pkg/R/functions.R Outdated Show resolved Hide resolved

MaxGekk added 2 commits July 21, 2019 00:13

Remove extra newlines

24fa82a

Rename @family

318cc18

Remove make_date from Scala, Python and R APIs

a5ef08b

dongjoon-hyun reviewed Jul 21, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 21, 2019

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/datetime.sql Outdated Show resolved Hide resolved

MaxGekk added 3 commits July 21, 2019 10:39

Remove quotes in the example

e92beea

Revert datetime.sql

8def6c3

Regen pgSQL/date.sql

30e62c2

MaxGekk commented Jul 21, 2019

View reviewed changes

dongjoon-hyun reviewed Jul 21, 2019

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/pgSQL/date.sql Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 21, 2019

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/pgSQL/date.sql Show resolved Hide resolved

Remove the comment: skip test constructors

646456b

gatorsmile reviewed Jul 21, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Show resolved Hide resolved

HyukjinKwon approved these changes Jul 22, 2019

View reviewed changes

ueshin approved these changes Jul 22, 2019

View reviewed changes

Add examples with invalid inputs

639c6b0

dongjoon-hyun reviewed Jul 22, 2019

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/pgSQL/date.sql Show resolved Hide resolved

Add a comment with JIRA ticket

15c64d2

dongjoon-hyun approved these changes Jul 22, 2019

View reviewed changes

dongjoon-hyun closed this in 2d74f14 Jul 22, 2019

MaxGekk mentioned this pull request Jul 24, 2019

[SPARK-28471][SQL] Replace yyyy by uuuu in date-timestamp patterns without era #25230

Closed

MaxGekk deleted the make_date-timestamp branch September 18, 2019 15:58

Conversation

MaxGekk commented Jul 20, 2019 • 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!

SparkQA commented Jul 20, 2019

Uh oh!

dongjoon-hyun commented Jul 20, 2019

Uh oh!

dongjoon-hyun commented Jul 20, 2019

Uh oh!

dongjoon-hyun commented Jul 20, 2019

Uh oh!

MaxGekk commented Jul 20, 2019

Uh oh!

MaxGekk commented Jul 20, 2019

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jul 20, 2019

Uh oh!

MaxGekk commented Jul 20, 2019

Uh oh!

SparkQA commented Jul 20, 2019

Uh oh!

dongjoon-hyun commented Jul 20, 2019

Uh oh!

dongjoon-hyun commented Jul 20, 2019

Uh oh!

SparkQA commented Jul 21, 2019

Uh oh!

MaxGekk commented Jul 21, 2019

Uh oh!

SparkQA commented Jul 21, 2019

Uh oh!

dongjoon-hyun commented Jul 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaxGekk Jul 21, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 21, 2019

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jul 21, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 21, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 21, 2019

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jul 21, 2019

Uh oh!

SparkQA commented Jul 21, 2019

Uh oh!

Uh oh!

gatorsmile commented Jul 21, 2019

Uh oh!

gatorsmile commented Jul 21, 2019

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

ueshin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxGekk commented Jul 20, 2019 •

edited

Loading

dongjoon-hyun commented Jul 21, 2019 •

edited

Loading