[SPARK-29623][SQL] do not allow multiple unit TO unit statements in interval literal syntax #26285

cloud-fan · 2019-10-28T16:01:57Z

What changes were proposed in this pull request?

re-arrange the parser rules to make it clear that multiple unit TO unit statement like SELECT INTERVAL '1-1' YEAR TO MONTH '2-2' YEAR TO MONTH is not allowed.

Why are the changes needed?

This is definitely an accident that we support such a weird syntax in the past. It's not supported by any other DBs and I can't think of any use case of it. Also no test covers this syntax in the current codebase.

Does this PR introduce any user-facing change?

Yes, and a migration guide item is added.

How was this patch tested?

new tests.

cloud-fan · 2019-10-28T16:03:07Z

cc @MaxGekk @dongjoon-hyun @maropu

cloud-fan · 2019-10-28T17:41:22Z

retest this please

shaneknapp · 2019-10-28T17:49:33Z

test this please

SparkQA · 2019-10-28T19:40:25Z

Test build #112787 has finished for PR 26285 at commit ca9073d.

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

maropu · 2019-10-28T23:34:08Z

retest this please

maropu · 2019-10-29T00:04:25Z

docs/sql-migration-guide.md


  - Since Spark 3.0, the `size` function returns `NULL` for the `NULL` input. In Spark version 2.4 and earlier, this function gives `-1` for the same input. To restore the behavior before Spark 3.0, you can set `spark.sql.legacy.sizeOfNull` to `true`.

+  - Since Spark 3.0, the interval literal syntax does not allow multiple unit TO unit statements anymore. For example, `SELECT INTERVAL '1-1' YEAR TO MONTH '2-2' YEAR TO MONTH'` throws parser exception.


super nit: I think its a little difficult to understand word boundaries "... multiple unit TO unit statements anymore.", so how about multiple unit-TO-unit statements anymore or multiple "unit TO unit" statements anymore

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

maropu · 2019-10-29T00:08:27Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

-    : {ansi}? INTERVAL? intervalField+
-    | {!ansi}? INTERVAL intervalField*
+    : {ansi}? INTERVAL? (errorCapturingMultiUnitsInterval | errorCapturingUnitToUnitInterval)
+    | {!ansi}? INTERVAL (errorCapturingMultiUnitsInterval | errorCapturingUnitToUnitInterval)


Just out of curiosity; is that bad to just drop + and * for simple fixes?

interval : {ansi}? INTERVAL? intervalField | {!ansi}? INTERVAL intervalField

intervalField can be single-unit statements like 1 year, which is allowed to repeat.

Oh, I see. Thanks!

maropu

+1 for dropping the support. LGTM except for minor comments.

SparkQA · 2019-10-29T01:19:31Z

Test build #112805 has finished for PR 26285 at commit ca9073d.

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

maropu · 2019-10-29T01:29:17Z

FYI: the test failure is not related to this pr: #26287

MaxGekk · 2019-10-29T15:21:30Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

+          case _ =>
+            throw new ParseException(s"Intervals $from TO $to are not supported.", ctx)
+        }
+        validate(interval != null, "No interval can be constructed", ctx)


Which of fromYearMonthString or fromDayTimeString can return null? They either throw an exception or return an interval.

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

MaxGekk · 2019-10-29T15:26:03Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

+          "Can only have a single unit TO unit statement in the interval literal syntax",
+          innerCtx.unitToUnitInterval)
+      }
+      Literal(visitMultiUnitsInterval(innerCtx.multiUnitsInterval))


Could you specify CalendarIntervalType to avoid unnecessary pattern matching for inferring the type by provided value.

SparkQA · 2019-10-29T19:53:23Z

Test build #112861 has finished for PR 26285 at commit 2b79f9c.

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

SparkQA · 2019-10-30T05:59:34Z

Test build #112886 has finished for PR 26285 at commit 9ab9554.

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

SparkQA · 2019-10-30T07:05:02Z

Test build #112885 has finished for PR 26285 at commit 940c0be.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ResolveCoalesceHints(conf: SQLConf) extends Rule[LogicalPlan]

cloud-fan · 2019-10-30T07:06:18Z

retest this please

SparkQA · 2019-10-30T08:57:13Z

Test build #112896 has finished for PR 26285 at commit 9ab9554.

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

cloud-fan · 2019-10-30T10:38:35Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

-    : {ansi}? INTERVAL? intervalField+
-    | {!ansi}? INTERVAL intervalField*
+    : INTERVAL (errorCapturingMultiUnitsInterval | errorCapturingUnitToUnitInterval)?
+    | {ansi}? (errorCapturingMultiUnitsInterval | errorCapturingUnitToUnitInterval)


{ansi}? (errorCapturingMultiUnitsInterval | errorCapturingUnitToUnitInterval)? is illegal as it can match anything. Note that we need to make (errorCapturingMultiUnitsInterval | errorCapturingUnitToUnitInterval) optional so that we can detect select interval and give precise error message.

cloud-fan · 2019-10-30T10:42:55Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/ExpressionParserSuite.scala


    // Unknown FROM TO intervals
-    intercept("interval 10 month to second",
+    intercept("interval '10' month to second",


from-to unit can only handle string values ('1-2' year to month, '1 12' day to hour).

If you do write interval 10 month to second, then we will ask users to use string instead, see https://github.com/apache/spark/pull/26285/files#diff-4f9e28af8e9fcb40a8a99b4e49f3b9b2R612

SparkQA · 2019-10-30T13:10:33Z

Test build #112918 has finished for PR 26285 at commit 51d9b45.

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

SparkQA · 2019-10-30T13:31:36Z

Test build #112916 has finished for PR 26285 at commit 356d19b.

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

SparkQA · 2019-10-30T13:41:48Z

Test build #112917 has finished for PR 26285 at commit 454b673.

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

cloud-fan · 2019-10-30T14:04:54Z

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

    })
-
-    val e1 = intercept[AnalysisException] {
-      sql("select interval")


moved to literals.sql

SparkQA · 2019-10-30T16:04:15Z

Test build #112928 has finished for PR 26285 at commit 9ec8f63.

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

SparkQA · 2019-10-30T22:00:16Z

Test build #112949 has finished for PR 26285 at commit e560cd8.

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

cloud-fan · 2019-10-31T07:44:45Z

retest this please

SparkQA · 2019-10-31T11:55:41Z

Test build #113005 has finished for PR 26285 at commit e560cd8.

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

maropu · 2019-10-31T12:00:18Z

retest this please

SparkQA · 2019-10-31T16:03:18Z

Test build #113027 has finished for PR 26285 at commit e560cd8.

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

cloud-fan · 2019-11-01T06:40:01Z

I believe the Spark R failure is not related. Any more comments before merging it?

SparkQA · 2019-11-01T16:53:35Z

Test build #113088 has finished for PR 26285 at commit 044257d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait HasRelativeError extends Params
class _ImputerParams(HasInputCol, HasInputCols, HasOutputCol, HasOutputCols, HasRelativeError):
class _RobustScalerParams(HasInputCol, HasOutputCol, HasRelativeError):
class HasRelativeError(Params):
case class AlterTableRenamePartitionStatement(
case class AlterTableDropPartitionStatement(
case class ShowColumnsStatement(
case class DataSourceV2ScanRelation(
case class OptimizeLocalShuffleReader(conf: SQLConf) extends Rule[SparkPlan]

cloud-fan · 2019-11-02T13:36:13Z

thanks for the review, merging to master!

cloud-fan mentioned this pull request Oct 28, 2019

[SPARK-29622][SQL] do not allow leading 'interval' in the interval string format #26283

Closed

maropu reviewed Oct 29, 2019

View reviewed changes

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

maropu reviewed Oct 29, 2019

View reviewed changes

MaxGekk reviewed Oct 29, 2019

View reviewed changes

cloud-fan added 2 commits October 30, 2019 01:31

do not allow multiple unit TO unit statements in interval literal syntax

66192ce

address comments

1aa9a0e

cloud-fan force-pushed the syntax branch from ca9073d to 1aa9a0e Compare October 29, 2019 17:43

refine

2b79f9c

Merge remote-tracking branch 'origin/master' into syntax

940c0be

cloud-fan force-pushed the syntax branch from 8761fa3 to 9ab9554 Compare October 30, 2019 05:27

dongjoon-hyun added the SQL label Oct 30, 2019

cloud-fan force-pushed the syntax branch from 9ab9554 to 356d19b Compare October 30, 2019 10:36

cloud-fan commented Oct 30, 2019

View reviewed changes

cloud-fan force-pushed the syntax branch from 356d19b to 454b673 Compare October 30, 2019 10:48

refine

51d9b45

cloud-fan force-pushed the syntax branch from 454b673 to 51d9b45 Compare October 30, 2019 10:58

cloud-fan commented Oct 30, 2019

View reviewed changes

fix test

e560cd8

cloud-fan force-pushed the syntax branch from 9ec8f63 to e560cd8 Compare October 30, 2019 17:53

MaxGekk approved these changes Nov 1, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into syntax

044257d

maropu approved these changes Nov 1, 2019

View reviewed changes

cloud-fan closed this in 31ae446 Nov 2, 2019


		- Since Spark 3.0, the `size` function returns `NULL` for the `NULL` input. In Spark version 2.4 and earlier, this function gives `-1` for the same input. To restore the behavior before Spark 3.0, you can set `spark.sql.legacy.sizeOfNull` to `true`.

		- Since Spark 3.0, the interval literal syntax does not allow multiple unit TO unit statements anymore. For example, `SELECT INTERVAL '1-1' YEAR TO MONTH '2-2' YEAR TO MONTH'` throws parser exception.

[SPARK-29623][SQL] do not allow multiple unit TO unit statements in interval literal syntax #26285

[SPARK-29623][SQL] do not allow multiple unit TO unit statements in interval literal syntax #26285

Uh oh!

Conversation

cloud-fan commented Oct 28, 2019

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?

Uh oh!

cloud-fan commented Oct 28, 2019

Uh oh!

cloud-fan commented Oct 28, 2019

Uh oh!

shaneknapp commented Oct 28, 2019

Uh oh!

SparkQA commented Oct 28, 2019

Uh oh!

maropu commented Oct 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maropu Oct 29, 2019 • 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!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 29, 2019

Uh oh!

maropu commented Oct 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 29, 2019

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

cloud-fan commented Oct 30, 2019

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Oct 30, 2019 • 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!

SparkQA commented Oct 30, 2019

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

cloud-fan commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

maropu commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

maropu Oct 29, 2019 •

edited

Loading

cloud-fan Oct 30, 2019 •

edited

Loading