[SPARK-29983][SQL] Add an independent config for an optional INTERVAL clause #26623

maropu · 2019-11-21T07:59:01Z

What changes were proposed in this pull request?

This pr is to add a new config only for an optional INTERVAL clause. In the master, this feature is enabled when spark.sql.ansi.enabled=true. This pr proposes to split off the optional interval flag from spark.sql.ansi.enabled.

This comes from the @cloud-fan suggestion: #26584 (comment)

Why are the changes needed?

Actually, an INTERVAL keyword is not optional in the ANSI/SQL standard.

<interval literal> ::=
  INTERVAL [ <sign> ] <interval string> <interval qualifier>

<interval string> ::=
  <quote> <unquoted interval string> <quote>

Does this PR introduce any user-facing change?

No.

How was this patch tested?

No.

maropu · 2019-11-21T07:59:12Z

cc: @cloud-fan @gengliangwang

cloud-fan · 2019-11-21T08:19:26Z

one correction: the SQL standard actually requires the INTERVAL prefix

<interval literal> ::=
  INTERVAL [ <sign> ] <interval string> <interval qualifier>

<interval string> ::=
  <quote> <unquoted interval string> <quote>

cloud-fan · 2019-11-21T08:21:33Z

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

  }

+  val OPTIONAL_INTERVAL =
+    buildConf("spark.sql.optionalInterval")


let's put it under the spark.sql.parser namespace

maropu · 2019-11-21T08:26:39Z

one correction: the SQL standard actually requires the INTERVAL prefix

Oh! I did misunderstand that.... thanks for the correction, @cloud-fan

maropu · 2019-11-21T08:30:15Z

The PR descripion updated based on the suggsetion above.

SparkQA · 2019-11-21T09:02:04Z

Test build #114223 has finished for PR 26623 at commit 6d0223e.

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

SparkQA · 2019-11-21T09:44:32Z

Test build #114215 has finished for PR 26623 at commit 5e20371.

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

cloud-fan · 2019-11-21T10:35:15Z

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

    lexer.addErrorListener(ParseErrorListener)
    lexer.legacy_setops_precedence_enbled = conf.setOpsPrecedenceEnforced
    lexer.SQL_standard_keyword_behavior = SQLStandardKeywordBehavior
+    lexer.optional_interval = conf.optionalInterval


I think it should be dialect == Spark && ansi == true && optionalInterval

For pgsql diaelct, INTERVAL can't be optional. for non-ansi mode, year/month/day... are not reserved keywords and we can't ommit INTERVAL prefix anyway.

cloud-fan · 2019-11-21T10:36:24Z

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

  }

+  val OPTIONAL_INTERVAL =
+    buildConf("spark.sql.parser.optionalInterval")


maybe "optionalIntervalPrefix"?

SparkQA · 2019-11-21T11:09:19Z

Test build #114235 has finished for PR 26623 at commit 02f54fd.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-21T11:24:54Z

Test build #114237 has finished for PR 26623 at commit 54a1f07.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-21T11:57:04Z

Test build #114228 has finished for PR 26623 at commit d53ec87.

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

maropu · 2019-11-21T12:20:15Z

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

    // into the head in this test.
-    val importedTestCaseName = comments.filter(_.startsWith("--IMPORT ")).map(_.substring(9))
-    val importedCode = importedTestCaseName.flatMap { testCaseName =>
+    val importedTestCaseNames = comments.filter(_.startsWith("--IMPORT ")).map(_.substring(9))


This is not related to this pr though, I found the misleading name here...

cloud-fan · 2019-11-21T14:01:41Z

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

@@ -1,17 +1,6 @@
+--SET spark.sql.parser.optionalIntervalPrefix=false


do we still need this file?

I just checked if we could turn off the optional prefix with ansi enabled. I personally think keeping this is better because the parser logic is a bit complicated for the combination about ansi + optionalIntervalPrefix...

SparkQA · 2019-11-21T14:07:36Z

Test build #114241 has finished for PR 26623 at commit 5868ef1.

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

SparkQA · 2019-11-21T14:38:09Z

Test build #114240 has finished for PR 26623 at commit 354b7a8.

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

SparkQA · 2019-11-22T02:11:10Z

Test build #114257 has finished for PR 26623 at commit 7e63c0f.

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

cloud-fan · 2019-11-22T03:29:37Z

sorry to have this proposal at this stage: can we drop this feature? It looks only useful when we have mysql/hive dialect someday, but I don't think it's going to happen in the near future. Even pgsql dialect is far away from completion.

maropu · 2019-11-23T00:23:29Z

ok

Fix

5e20371

cloud-fan reviewed Nov 21, 2019

View reviewed changes

Fix

6d0223e

Fix

d53ec87

cloud-fan reviewed Nov 21, 2019

View reviewed changes

maropu force-pushed the OptinalInterval branch from 02f54fd to 54a1f07 Compare November 21, 2019 11:11

maropu force-pushed the OptinalInterval branch 2 times, most recently from 354b7a8 to a23ee2c Compare November 21, 2019 12:16

Fix

5868ef1

maropu force-pushed the OptinalInterval branch from a23ee2c to 5868ef1 Compare November 21, 2019 12:18

maropu commented Nov 21, 2019

View reviewed changes

cloud-fan reviewed Nov 21, 2019

View reviewed changes

dongjoon-hyun added the SQL label Nov 21, 2019

Fix

7e63c0f

maropu closed this Nov 23, 2019

		@@ -1,17 +1,6 @@
		--SET spark.sql.parser.optionalIntervalPrefix=false

[SPARK-29983][SQL] Add an independent config for an optional INTERVAL clause #26623

[SPARK-29983][SQL] Add an independent config for an optional INTERVAL clause #26623

Uh oh!

Conversation

maropu commented Nov 21, 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?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

maropu commented Nov 21, 2019

Uh oh!

cloud-fan commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

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!

SparkQA commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

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!

SparkQA commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

cloud-fan commented Nov 22, 2019

Uh oh!

maropu commented Nov 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maropu commented Nov 21, 2019 •

edited

Loading

maropu commented Nov 21, 2019 •

edited

Loading