[SPARK-27638][SQL]: Cast string to date/timestamp in binary comparisons with dates/timestamps #24567

pengbo · 2019-05-09T12:54:37Z

What changes were proposed in this pull request?

The below example works with both Mysql and Hive, however not with spark.

mysql> select * from date_test where date_col >= '2000-1-1';
+------------+
| date_col   |
+------------+
| 2000-01-01 |
+------------+

The reason is that Spark casts both sides to String type during date and string comparison for partial date support. Please find more details in https://issues.apache.org/jira/browse/SPARK-8420.

Based on some tests, the behavior of Date and String comparison in Hive and Mysql:
Hive: Cast to Date, partial date is not supported
Mysql: Cast to Date, certain "partial date" is supported by defining certain date string parse rules. Check out str_to_datetime in https://github.com/mysql/mysql-server/blob/5.5/sql-common/my_time.c

As below date patterns have been supported, the PR is to cast string to date when comparing string and date:

`yyyy`
`yyyy-[m]m`
`yyyy-[m]m-[d]d`
`yyyy-[m]m-[d]d `
`yyyy-[m]m-[d]d *`
`yyyy-[m]m-[d]dT*

How was this patch tested?

UT has been added

…erly

pengbo · 2019-05-09T12:59:29Z

cc @cloud-fan @MaxGekk

Some corner cases like date_col > 'invalid_date_string' will always return false in this solution seems to be reasonable as well In my opinion such as date_col > '2000-' or '19999' (the same behavior in Mysql).

HyukjinKwon · 2019-05-09T14:04:47Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

-    case (StringType, DateType) => Some(StringType)
-    case (DateType, StringType) => Some(StringType)
+    case (StringType, DateType) => Some(DateType)
+    case (DateType, StringType) => Some(DateType)


Doesn't this mean we always find the common type as date when any arbitrary strings are compared to any dates?

Yes, it there any issue in your opinion?

HyukjinKwon · 2019-05-09T14:05:34Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

@@ -123,8 +123,8 @@ object TypeCoercion {
    // We should cast all relative timestamp/date/string comparison into string comparisons
    // This behaves as a user would expect because timestamp strings sort lexicographically.
    // i.e. TimeStamp(2013-01-01 00:00 ...) < "2014" = true


Looks we should update this comments too.

Done. Thanks for pointing it out

cloud-fan · 2019-05-09T15:01:29Z

Some corner cases like date_col > 'invalid_date_string' will always return false in this solution seems to be reasonable

It will be super weird if date_col > 'invalid_date_string', date_col < 'invalid_date_string' and date_col = 'invalid_date_string' all return false. I think returning null is more reasonable.

pengbo · 2019-05-10T02:19:24Z

Some corner cases like date_col > 'invalid_date_string' will always return false in this solution seems to be reasonable

It will be super weird if date_col > 'invalid_date_string', date_col < 'invalid_date_string' and date_col = 'invalid_date_string' all return false. I think returning null is more reasonable.

It returns null indeed. Sorry for any misunderstanding

…7638

wangyum

This change discussed with @gatorsmile before, he has a long-term planning.

cloud-fan · 2019-05-10T14:54:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

-    // We should cast all relative timestamp/date/string comparison into string comparisons
+    case (StringType, DateType) => Some(DateType)
+    case (DateType, StringType) => Some(DateType)
+    // We should cast all relative timestamp/string comparison into string comparisons


if it's justified to do it for date, I think we should do it for timestamp as well.

MaxGekk · 2019-05-12T08:50:17Z

@pengbo Could you change the title of the PR to reflect the change. For example:

[SPARK-27638][SQL] Cast string to date in binary comparisons with dates

MaxGekk · 2019-05-12T08:53:24Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

@@ -3024,6 +3024,20 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
      sql("reset")
    }
  }
+
+  test("string date comparison") {


Please, add test cases for <=>, =

MaxGekk · 2019-05-12T09:10:44Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

@@ -120,11 +120,11 @@ object TypeCoercion {
   */
  private def findCommonTypeForBinaryComparison(
      dt1: DataType, dt2: DataType, conf: SQLConf): Option[DataType] = (dt1, dt2) match {
-    // We should cast all relative timestamp/date/string comparison into string comparisons
+    case (StringType, DateType) => Some(DateType)


I have removed these 2 cases but the added test still completes successfully. We need a few tests that fail without the changes in findCommonTypeForBinaryComparison.

It is explained by https://github.com/apache/spark/pull/24567/files#issuecomment-491579283

Done.
Thanks for pointing it out. It simplifies the code a lot.

MaxGekk · 2019-05-12T09:21:28Z

It seems the updated cases:

case (StringType, DateType) => Some(DateType)
case (DateType, StringType) => Some(DateType)

are sub-set of other cases in the same method

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

Lines 147 to 148 in 72a572f

    
           case (l: StringType, r: AtomicType) if r != StringType => Some(r) 
        
           case (l: AtomicType, r: StringType) if l != StringType => Some(l)

I think the cases for pairs of StringType and DateType can be just removed.

…cases

pengbo · 2019-05-12T14:52:18Z

@pengbo Could you change the title of the PR to reflect the change. For example:
[SPARK-27638][SQL] Cast string to date in binary comparisons with dates

Done

MaxGekk · 2019-05-12T15:20:23Z

@cloud-fan @HyukjinKwon Could you trigger build & tests.

kiszk · 2019-05-12T17:52:09Z

ok to test

SparkQA · 2019-05-12T20:01:07Z

Test build #105342 has finished for PR 24567 at commit ac82eb2.

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

cloud-fan · 2019-05-13T07:01:55Z

A general note: eventually we do need to support different type coercion modes for different users. But the type coercion rule should be reasonable under any mode. It's not reasonable to compare date/timestamp and string by casting to string, as it returns the wrong result when the string is a partial date/timestamp.

For safety, we can add a legacy config to fallback to the old behavior.

…tamp comparison behavior & failed unit tests correction

…7638

cloud-fan · 2019-05-13T09:13:41Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

@@ -1760,6 +1760,13 @@ object SQLConf {
    .internal()
    .intConf
    .createWithDefault(Int.MaxValue)
+
+  val LEGACY_CAST_DATE_TIMESTAMP_TO_STRING =
+    buildConf("spark.sql.legacy.binaryComparison.castDateTimestampToString")


spark.sql.legacy.typeCoercion.datetimeToString

cloud-fan · 2019-05-13T09:14:33Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+
+  val LEGACY_CAST_DATE_TIMESTAMP_TO_STRING =
+    buildConf("spark.sql.legacy.binaryComparison.castDateTimestampToString")
+      .doc("If it is set to true, date/timestamp will cast to string in binary comparisons " +


can we also add a migration guide for this behavior change and mention this config?

pengbo · 2019-05-13T10:36:45Z

retest it please

SparkQA · 2019-05-13T12:11:12Z

Test build #105352 has finished for PR 24567 at commit 34b00e9.

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

cloud-fan · 2019-05-13T12:28:07Z

LGTM

SparkQA · 2019-05-13T13:33:29Z

Test build #105354 has finished for PR 24567 at commit c3e80f7.

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

cloud-fan · 2019-05-14T09:11:30Z

thanks, merging to master!

gatorsmile · 2020-01-01T23:54:01Z

docs/sql-migration-guide-upgrade.md

@@ -128,6 +128,8 @@ license: |

  - Since Spark 3.0, if `hive.default.fileformat` is not found in `Spark SQL configuration` then it will fallback to hive-site.xml present in the `Hadoop configuration` of `SparkContext`.

+  - Since Spark 3.0, Spark will cast `String` to `Date/TimeStamp` in binary comparisons with dates/timestamps. The previous behaviour of casting `Date/Timestamp` to `String` can be restored by setting `spark.sql.legacy.typeCoercion.datetimeToString` to `true`.


spark.sql.legacy.typeCoercion.datetimeToString -> spark.sql.legacy.typeCoercion.datetimeToString.enabled

cc @Ngone51

SPARK-27638: date format yyyy-M-dd string comparison not handled prop…

e24514e

…erly

HyukjinKwon reviewed May 9, 2019

View reviewed changes

mingbo.pb added 2 commits May 10, 2019 10:23

change relative comments

41a03dd

Merge branch 'master' of https://github.com/apache/spark into SPARK-2…

19ec071

…7638

wangyum reviewed May 10, 2019

View reviewed changes

cloud-fan reviewed May 10, 2019

View reviewed changes

MaxGekk reviewed May 12, 2019

View reviewed changes

cast to timestamp when comparting string and timestamp & enrich test …

ac82eb2

…cases

pengbo changed the title ~~[SPARK-27638][SQL]: date format 'yyyy-M-dd' string comparison not handled properly~~ [SPARK-27638][SQL]: Cast string to date/timestamp in binary comparisons with dates/timestamps May 12, 2019

mingbo.pb added 2 commits May 13, 2019 17:01

add SQLConf legacy option to fallback to the old string vs date/times…

34b00e9

…tamp comparison behavior & failed unit tests correction

Merge branch 'master' of https://github.com/apache/spark into SPARK-2…

e63367c

…7638

cloud-fan reviewed May 13, 2019

View reviewed changes

Modify migration guide & rename SQLConf option

c3e80f7

MaxGekk approved these changes May 13, 2019

View reviewed changes

cloud-fan closed this in 66f5a42 May 14, 2019

gatorsmile reviewed Jan 1, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-27638][SQL]: Cast string to date/timestamp in binary comparisons with dates/timestamps #24567

[SPARK-27638][SQL]: Cast string to date/timestamp in binary comparisons with dates/timestamps #24567

pengbo commented May 9, 2019

pengbo commented May 9, 2019

HyukjinKwon May 9, 2019

pengbo May 10, 2019

HyukjinKwon May 9, 2019

pengbo May 10, 2019

cloud-fan commented May 9, 2019

pengbo commented May 10, 2019 •

edited

wangyum left a comment

cloud-fan May 10, 2019

pengbo May 12, 2019

MaxGekk commented May 12, 2019

MaxGekk May 12, 2019

pengbo May 12, 2019

MaxGekk May 12, 2019

MaxGekk May 12, 2019

pengbo May 12, 2019

MaxGekk commented May 12, 2019

pengbo commented May 12, 2019

MaxGekk commented May 12, 2019

kiszk commented May 12, 2019

SparkQA commented May 12, 2019

cloud-fan commented May 13, 2019

cloud-fan May 13, 2019

cloud-fan May 13, 2019

pengbo commented May 13, 2019

SparkQA commented May 13, 2019

cloud-fan commented May 13, 2019

SparkQA commented May 13, 2019

cloud-fan commented May 14, 2019

gatorsmile Jan 1, 2020

gatorsmile Jan 1, 2020

Ngone51 Jan 2, 2020

		@@ -128,6 +128,8 @@ license: \|

		- Since Spark 3.0, if `hive.default.fileformat` is not found in `Spark SQL configuration` then it will fallback to hive-site.xml present in the `Hadoop configuration` of `SparkContext`.

		- Since Spark 3.0, Spark will cast `String` to `Date/TimeStamp` in binary comparisons with dates/timestamps. The previous behaviour of casting `Date/Timestamp` to `String` can be restored by setting `spark.sql.legacy.typeCoercion.datetimeToString` to `true`.

[SPARK-27638][SQL]: Cast string to date/timestamp in binary comparisons with dates/timestamps #24567

[SPARK-27638][SQL]: Cast string to date/timestamp in binary comparisons with dates/timestamps #24567

Conversation

pengbo commented May 9, 2019

What changes were proposed in this pull request?

How was this patch tested?

pengbo commented May 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented May 9, 2019

pengbo commented May 10, 2019 • edited

wangyum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk commented May 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk commented May 12, 2019

pengbo commented May 12, 2019

MaxGekk commented May 12, 2019

kiszk commented May 12, 2019

SparkQA commented May 12, 2019

cloud-fan commented May 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pengbo commented May 13, 2019

SparkQA commented May 13, 2019

cloud-fan commented May 13, 2019

SparkQA commented May 13, 2019

cloud-fan commented May 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pengbo commented May 10, 2019 •

edited