[SPARK-29783][SQL] Support SQL Standard/ISO_8601 output style for interval type #26418

yaooqinn · 2019-11-07T03:14:06Z

What changes were proposed in this pull request?

Add 3 interval output types which are named as SQL_STANDARD, ISO_8601, MULTI_UNITS. And we add a new conf spark.sql.dialect.intervalOutputStyle for this. The MULTI_UNITS style displays the interval values in the former behavior and it is the default. The newly added SQL_STANDARD, ISO_8601 styles can be found in the following table.

Style	conf	Year-Month Interval	Day-Time Interval	Mixed Interval
Format With Time Unit Designators	MULTI_UNITS	1 year 2 mons	1 days 2 hours 3 minutes 4.123456 seconds	interval 1 days 2 hours 3 minutes 4.123456 seconds
SQL STANDARD	SQL_STANDARD	1-2	3 4:05:06	-1-2 3 -4:05:06
ISO8601 Basic Format	ISO_8601	P1Y2M	P3DT4H5M6S	P-1Y-2M3D-4H-5M-6S

Why are the changes needed?

for ANSI SQL support

Does this PR introduce any user-facing change?

yes，interval out now has 3 output styles

How was this patch tested?

add new unit tests

cc @cloud-fan @maropu @MaxGekk @HyukjinKwon thanks.

SparkQA · 2019-11-07T03:46:40Z

Test build #113355 has finished for PR 26418 at commit 88418e0.

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

cloud-fan · 2019-11-07T05:01:37Z

can you check with some other databases?

yaooqinn · 2019-11-07T05:59:45Z

1. vertica

https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/SQLReferenceManual/DataTypes/Date-Time/DisplayingOrOmittingIntervalUnitsInOutput.htm?tocpath=SQL%20Reference%20Manual%7CSQL%20Data%20Types%7CDate%2FTime%20Data%20Types%7CINTERVAL%7C_____1

If DATESTYLE is set to SQL, interval unit display always conforms to the SQL:2008 standard, which omits interval unit display. If DATESTYLE is set to ISO, you can use SET INTERVALSTYLE to omit or display interval unit display.

2. mysql

MySQL seems to have intervals but not interval types, using time and year as an alternative.
https://dev.mysql.com/doc/refman/8.0/en/year.html
https://dev.mysql.com/doc/refman/8.0/en/time.html

3. Postgres

https://www.postgresql.org/docs/9.0/datatype-datetime.html#DATATYPE-INTERVAL-OUTPUT

Pg has 4 output styles for intervals. sql_standard, postgres, postgres_verbose, and iso_8601

4 presto

no so clear on the online doc https://prestodb.github.io/docs/current/language/types.html#date-and-time, verifed by manual

presto> select INTERVAL '3' DAY;
     _col0
----------------
 3 00:00:00.000
(1 row)

Query 20191107_060825_00008_n4kma, FINISHED, 1 node
Splits: 17 total, 17 done (100.00%)
0:01 [0 rows, 0B] [0 rows/s, 0B/s]

presto> select INTERVAL '3' MONTH;
 _col0
-------
 0-3
(1 row)

Query 20191107_060834_00009_n4kma, FINISHED, 1 node
Splits: 17 total, 17 done (100.00%)
0:00 [0 rows, 0B] [0 rows/s, 0B/s]

@cloud-fan Currently I have checked for these dbs. thanks

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

SparkQA · 2019-11-07T07:28:54Z

Test build #113368 has finished for PR 26418 at commit 02be22b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class LocalShuffleReaderExec(child: SparkPlan) extends UnaryExecNode
class ContinuousRecordEndpoint(buckets: Seq[Seq[UnsafeRow]], lock: Object)

SparkQA · 2019-11-07T08:05:01Z

Test build #113366 has finished for PR 26418 at commit 60441d5.

This patch fails due to an unknown error code, -9.
This patch does not merge cleanly.
This patch adds no public classes.

cloud-fan · 2019-11-07T08:45:41Z

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

@@ -409,6 +409,8 @@ case class Literal (value: Any, dataType: DataType) extends LeafExpression {
        DateTimeUtils.getZoneId(SQLConf.get.sessionLocalTimeZone))
      s"TIMESTAMP('${formatter.format(v)}')"
    case (v: Array[Byte], BinaryType) => s"X'${DatatypeConverter.printHexBinary(v)}'"
+    case (v: CalendarInterval, CalendarIntervalType) if SQLConf.get.ansiEnabled =>
+      IntervalUtils.toSqlStandardString(v)


This should be something that can be parsed, I think we need to output something like INTERVAL'1 year 2 days'

We don't support SQL standard input for intervals? I missed that, may cause user behavior change. But If we stay multi-unit style here, would there be conflicting between literals and other exprs？

changed back , thanks

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

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

yaooqinn · 2019-11-07T15:40:20Z

I add a new config to control this output behavior, since ansi switch is too dressy for this.

SparkQA · 2019-11-07T16:39:12Z

Test build #113383 has finished for PR 26418 at commit fff17d5.

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

maropu · 2019-11-08T13:25:48Z

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

+    .stringConf
+    .transform(_.toUpperCase(Locale.ROOT))
+    .checkValues(IntervalStyle.values.map(_.toString))
+    .createWithDefault(IntervalStyle.MULTI_UNITS.toString)


I personally think ansiEnabled is enough for this feature. Any concern?

yes, I guess some users may already rely on the output string

MaxGekk · 2019-11-11T16:54:15Z

You have to change the code gen in castToStringCode as well.

SparkQA · 2019-11-11T20:50:12Z

Test build #113597 has finished for PR 26418 at commit 005a28d.

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

yaooqinn · 2019-11-12T02:34:42Z

You have to change the code gen in castToStringCode as well.

thanks @MaxGekk

SparkQA · 2019-11-12T04:32:52Z

Test build #113607 has finished for PR 26418 at commit fa9c41e.

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

cloud-fan · 2019-11-13T15:32:33Z

@yaooqinn can you revert the json side changes?

yaooqinn · 2019-11-13T16:26:03Z

@yaooqinn can you revert the json side changes?

the changes in JacksonGenerator.scala should be kept from consistency, cause interval's toString has changed. I will rm the others.

cloud-fan · 2019-11-13T16:41:34Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonGenerator.scala

@@ -214,10 +218,15 @@ private[sql] class JacksonGenerator(
  private def writeMapData(
      map: MapData, mapType: MapType, fieldWriter: ValueWriter): Unit = {
    val keyArray = map.keyArray()
+    val keyString = mapType.keyType match {
+      case CalendarIntervalType =>


This can't happen actually. We don't allow writing out interval values. Do you have an example that can hit this code path?

3aa723c#diff-8a461909033e3f735f8239825970c378R218-L229

ah to_json do not have the interval type check. makes sense.

@yaooqinn, how about to_csv?

cloud-fan · 2019-11-13T17:13:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JSONOptions.scala

@@ -26,6 +26,8 @@ import com.fasterxml.jackson.core.{JsonFactory, JsonParser}
 import org.apache.spark.internal.Logging
 import org.apache.spark.sql.catalyst.util._
 import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.internal.SQLConf.IntervalStyle
+import org.apache.spark.sql.internal.SQLConf.IntervalStyle.IntervalStyle


unused import

cloud-fan · 2019-11-13T17:15:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonGenerator.scala

+    val keyString = mapType.keyType match {
+      case CalendarIntervalType =>
+        (i: Int) => IntervalUtils.toMultiUnitsString(keyArray.getInterval(i))
+      case _ => (i: Int) => keyArray.get(i, mapType.keyType).toString


it's fragile to rely on toString. e.g. UnsafeRow.toString is not human readable. Shall we recursively write map key as json object? cc @HyukjinKwon

Ah, I am sorry I missed this cc. in JSON the key should be a string. We should either make it string always or explicitly disallow.

cc @viirya I think we talked about this before.

Yea, I think currently the map key is not very useful for some types. To make human readable map keys, we need do specific serialization for some map key types. Maybe I create a JIRA ticket to follow it up?

Yeah .. +1 !

Created https://issues.apache.org/jira/browse/SPARK-29946 to follow it up.

SparkQA · 2019-11-13T19:21:48Z

Test build #113717 has finished for PR 26418 at commit 8d18fac.

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

SparkQA · 2019-11-13T20:13:58Z

Test build #113719 has finished for PR 26418 at commit 0c530cc.

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

cloud-fan · 2019-11-16T09:57:53Z

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

@@ -88,3 +88,46 @@ select justify_interval(interval '1 month -59 day 25 hour');
 select justify_days(interval '1 month 59 day -25 hour');
 select justify_hours(interval '1 month 59 day -25 hour');
 select justify_interval(interval '1 month 59 day -25 hour');
+
+-- interval output style


we can create a interval-display.sql test file to put these test cases, and then a interval-display-sql-standard.sql which does

--SET spark.sql.intervalOutputStyle = SQL_STANDARD; --import interval-display.sql

also a interval-display-iso_8601.sql

--SET spark.sql.intervalOutputStyle = SQL_STANDARD --import interval-display.sql

these 2 don't fit each other well,
When regenerating golden files, the set operations will not happen, considering this as a bug,
also cc @maropu

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

Lines 288 to 295 in 40ea4a1

if (regenerateGoldenFiles || !isTestWithConfigSets) {

runQueries(queries, testCase, None)

} else {

val configSets = {

val configLines = comments.filter(_.startsWith("--SET")).map(_.substring(5))

val configs = configLines.map(_.split(",").map { confAndValue =>

val (conf, value) = confAndValue.span(_ != '=')

conf.trim -> value.substring(1).trim

In this PR, the golden files are generated with this fix #26557, because it has nothing to do with this PR, so I just raised a follow-up to the ticket SPARK-29873 which involved this feature.

SparkQA · 2019-11-16T14:25:21Z

Test build #113927 has finished for PR 26418 at commit 7641e5e.

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

SparkQA · 2019-11-16T18:16:28Z

Test build #113936 has finished for PR 26418 at commit 0f54af8.

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

cloud-fan · 2019-11-18T07:42:38Z

thanks, merging to master!

HyukjinKwon · 2019-11-18T08:47:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonGenerator.scala

+    val keyString = mapType.keyType match {
+      case CalendarIntervalType =>
+        (i: Int) => IntervalUtils.toMultiUnitsString(keyArray.getInterval(i))
+      case _ => (i: Int) => keyArray.get(i, mapType.keyType).toString


This code path shouldn't be here per each map here BTW.

HyukjinKwon · 2019-11-18T08:56:20Z

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

@@ -409,6 +409,7 @@ case class Literal (value: Any, dataType: DataType) extends LeafExpression {
        DateTimeUtils.getZoneId(SQLConf.get.sessionLocalTimeZone))
      s"TIMESTAMP('${formatter.format(v)}')"
    case (v: Array[Byte], BinaryType) => s"X'${DatatypeConverter.printHexBinary(v)}'"
+    case (v: CalendarInterval, CalendarIntervalType) => IntervalUtils.toMultiUnitsString(v)


Sorry if this is already asked above but why we didn't change this?

We have not supported to parse interval from iso/SQL standard format yet

Why did we not support iso/SQL standard format here together?

#26418 (comment)

HyukjinKwon · 2019-11-18T09:06:13Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

-      sb.append(value).append(' ').append(unit).append(' ');
-    }
+    return "CalendarInterval(months= " + months + ", days = " + days + ", microsecond = " +
+      microseconds + ")";


Why did we use such string representation now? Was it in order to put the same logics into IntervalUtils? If that's the case, we didn't have to move but use toString of this class until this case becomes completely exposed.

HyukjinKwon · 2019-11-18T09:16:09Z

Let me make a followup by myself.

HyukjinKwon · 2019-11-18T09:38:24Z

Made #26572

HyukjinKwon · 2019-11-18T09:42:39Z

...erver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala

@@ -93,7 +93,10 @@ class ThriftServerQueryTestSuite extends SQLQueryTestSuite {
    "subquery/in-subquery/in-group-by.sql",
    "subquery/in-subquery/simple-in.sql",
    "subquery/in-subquery/in-order-by.sql",
-    "subquery/in-subquery/in-set-operations.sql"
+    "subquery/in-subquery/in-set-operations.sql",
+    // SPARK-29783: need to set conf


Was it only because we couldn't set the configurations? If Thrift server does not respect configuration like:

--SET spark.sql.intervalOutputStyle = ISO_8601

it looks a bug to me. @wangyum

We can remove this config:

spark/sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala

Line 78 in 3163b6b

override val isTestWithConfigSets = false

### What changes were proposed in this pull request? This is a followup of #26418. This PR removed `CalendarInterval`'s `toString` with an unfinished changes. ### Why are the changes needed? 1. Ideally we should make each PR isolated and separate targeting one issue without touching unrelated codes. 2. There are some other places where the string formats were exposed to users. For example: ```scala scala> sql("select interval 1 days as a").selectExpr("to_csv(struct(a))").show() ``` ``` +--------------------------+ |to_csv(named_struct(a, a))| +--------------------------+ | "CalendarInterval...| +--------------------------+ ``` 3. Such fixes: ```diff private def writeMapData( map: MapData, mapType: MapType, fieldWriter: ValueWriter): Unit = { val keyArray = map.keyArray() + val keyString = mapType.keyType match { + case CalendarIntervalType => + (i: Int) => IntervalUtils.toMultiUnitsString(keyArray.getInterval(i)) + case _ => (i: Int) => keyArray.get(i, mapType.keyType).toString + } ``` can cause performance regression due to type dispatch for each map. ### Does this PR introduce any user-facing change? Yes, see 2. case above. ### How was this patch tested? Manually tested. Closes #26572 from HyukjinKwon/SPARK-29783. Authored-by: HyukjinKwon <gurwls223@apache.org> Signed-off-by: HyukjinKwon <gurwls223@apache.org>

… we decide not to follow ANSI and no round trip ### What changes were proposed in this pull request? This revert #26418, file a new ticket under https://issues.apache.org/jira/browse/SPARK-30546 for better tracking interval behavior ### Why are the changes needed? Revert interval ISO/ANSI SQL Standard output since we decide not to follow ANSI and there is no round trip ### Does this PR introduce any user-facing change? no, not released yet ### How was this patch tested? existing uts Closes #27304 from yaooqinn/SPARK-30593. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

[SPARK-29783][SQL] Support SQL Standard output style for interval type

88418e0

MaxGekk reviewed Nov 7, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala Outdated Show resolved Hide resolved

Merge branch 'master' into SPARK-29783

02be22b

cloud-fan reviewed Nov 7, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala Show resolved Hide resolved

yaooqinn added 3 commits November 7, 2019 18:16

Merge branch 'master' into SPARK-29783

429ee49

Merge branch 'master' into SPARK-29783

da119c5

add a sep conf

fff17d5

yaooqinn commented Nov 7, 2019

View reviewed changes

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

maropu reviewed Nov 8, 2019

View reviewed changes

dongjoon-hyun added the SQL label Nov 9, 2019

yaooqinn requested review from maropu and cloud-fan November 11, 2019 05:36

Merge remote-tracking branch 'origin/master' into SPARK-29783

2af8593

yaooqinn changed the title ~~[SPARK-29783][SQL] Support SQL Standard output style for interval type~~ [SPARK-29783][SQL] Support SQL Standard/ISO_8601 output style for interval type Nov 11, 2019

yaooqinn added 2 commits November 12, 2019 00:42

add support for iso_8601 output

b3c9e08

add ut

005a28d

mv calendar interval toString to utils

5ea0ff8

fix java style

fa9c41e

cloud-fan reviewed Nov 13, 2019

View reviewed changes

rm json part change and change config back

8d18fac

cloud-fan reviewed Nov 13, 2019

View reviewed changes

import

0c530cc

cloud-fan reviewed Nov 16, 2019

View reviewed changes

yaooqinn added 2 commits November 16, 2019 18:50

Merge remote-tracking branch 'origin/master' into SPARK-29783

08b7359

create interval-display*.sql

7641e5e

blacklisting new tests in thrift test

0f54af8

cloud-fan closed this in 5cebe58 Nov 18, 2019

HyukjinKwon reviewed Nov 18, 2019

View reviewed changes

HyukjinKwon mentioned this pull request Nov 18, 2019

[SPARK-29870][SQL][FOLLOW-UP] Keep CalendarInterval's toString #26572

Closed

HyukjinKwon reviewed Nov 18, 2019

View reviewed changes

yaooqinn mentioned this pull request Jan 21, 2020

[SPARK-30593][SQL] Revert interval ISO/ANSI SQL Standard output since we decide not to follow ANSI and no round trip #27304

Closed

	if (regenerateGoldenFiles \|\| !isTestWithConfigSets) {
	runQueries(queries, testCase, None)
	} else {
	val configSets = {
	val configLines = comments.filter(_.startsWith("--SET")).map(_.substring(5))
	val configs = configLines.map(_.split(",").map { confAndValue =>
	val (conf, value) = confAndValue.span(_ != '=')
	conf.trim -> value.substring(1).trim

[SPARK-29783][SQL] Support SQL Standard/ISO_8601 output style for interval type #26418

[SPARK-29783][SQL] Support SQL Standard/ISO_8601 output style for interval type #26418

Conversation

yaooqinn commented Nov 7, 2019 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Nov 7, 2019

cloud-fan commented Nov 7, 2019

yaooqinn commented Nov 7, 2019 • edited

1. vertica

2. mysql

3. Postgres

4 presto

SparkQA commented Nov 7, 2019

SparkQA commented Nov 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn commented Nov 7, 2019

SparkQA commented Nov 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk commented Nov 11, 2019

SparkQA commented Nov 11, 2019

yaooqinn commented Nov 12, 2019

SparkQA commented Nov 12, 2019

cloud-fan commented Nov 13, 2019

yaooqinn commented Nov 13, 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

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

Choose a reason for hiding this comment

SparkQA commented Nov 13, 2019

SparkQA commented Nov 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn Nov 16, 2019 • edited

Choose a reason for hiding this comment

SparkQA commented Nov 16, 2019

SparkQA commented Nov 16, 2019

cloud-fan commented Nov 18, 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

Choose a reason for hiding this comment

HyukjinKwon commented Nov 18, 2019

HyukjinKwon commented Nov 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn commented Nov 7, 2019 •

edited

yaooqinn commented Nov 7, 2019 •

edited

yaooqinn Nov 16, 2019 •

edited