[SPARK-25979][SQL] Window function: allow parentheses around window reference #22987

gengliangwang · 2018-11-08T16:23:29Z

What changes were proposed in this pull request?

Very minor parser bug, but possibly problematic for code-generated queries:

Consider the following two queries:

SELECT avg(k) OVER (w) FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1

and

SELECT avg(k) OVER w FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1

The former, with parens around the OVER condition, fails to parse while the latter, without parens, succeeds:

Error in SQL statement: ParseException: 
mismatched input '(' expecting {<EOF>, ',', 'FROM', 'WHERE', 'GROUP', 'ORDER', 'HAVING', 'LIMIT', 'LATERAL', 'WINDOW', 'UNION', 'EXCEPT', 'MINUS', 'INTERSECT', 'SORT', 'CLUSTER', 'DISTRIBUTE'}(line 1, pos 19)

== SQL ==
SELECT avg(k) OVER (w) FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1
-------------------^^^

This was found when running the cockroach DB tests.

I tried PostgreSQL, The SQL with parentheses is also workable.

How was this patch tested?

Unit test

gengliangwang · 2018-11-08T16:25:48Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLWindowFunctionSuite.scala

@@ -31,6 +32,19 @@ class SQLWindowFunctionSuite extends QueryTest with SharedSQLContext {

  import testImplicits._

+  val empSalaryData = Seq(


The table and data here is from: https://www.postgresql.org/docs/9.1/tutorial-window.html

The naming and data is easier to be understood. In case we need to compare more behavior, we can use this one as alternative.
I can use the WindowData and remove this as well.

can we just add a new test in window.sql?

We really just need a simple test that proves over (w) works.

gengliangwang · 2018-11-08T16:30:39Z

@gatorsmile @cloud-fan

SparkQA · 2018-11-08T19:57:42Z

Test build #98611 has finished for PR 22987 at commit c9d01df.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class EmpSalary(empno: Int, depname: String, salary: Double)

cloud-fan · 2018-11-09T03:37:31Z

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

+SELECT cate, sum(val) OVER (w)
+FROM testData
+WHERE val is not null
+WINDOW w AS (PARTITION BY cate ORDER BY val);


need a new line at the end.

gatorsmile · 2018-11-09T03:53:22Z

LGTM

SparkQA · 2018-11-09T05:33:28Z

Test build #98633 has finished for PR 22987 at commit 2da6f99.

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

SparkQA · 2018-11-09T07:54:12Z

Test build #98636 has finished for PR 22987 at commit 471092d.

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

…eference ## What changes were proposed in this pull request? Very minor parser bug, but possibly problematic for code-generated queries: Consider the following two queries: ``` SELECT avg(k) OVER (w) FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1 ``` and ``` SELECT avg(k) OVER w FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1 ``` The former, with parens around the OVER condition, fails to parse while the latter, without parens, succeeds: ``` Error in SQL statement: ParseException: mismatched input '(' expecting {<EOF>, ',', 'FROM', 'WHERE', 'GROUP', 'ORDER', 'HAVING', 'LIMIT', 'LATERAL', 'WINDOW', 'UNION', 'EXCEPT', 'MINUS', 'INTERSECT', 'SORT', 'CLUSTER', 'DISTRIBUTE'}(line 1, pos 19) == SQL == SELECT avg(k) OVER (w) FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1 -------------------^^^ ``` This was found when running the cockroach DB tests. I tried PostgreSQL, The SQL with parentheses is also workable. ## How was this patch tested? Unit test Closes #22987 from gengliangwang/windowParentheses. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com> (cherry picked from commit 1db7997) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

gatorsmile · 2018-11-09T17:46:43Z

LGTM

Thanks! Merged to master.

I accidentally merged it to 2.4. Feel free to revert it if anybody has a concern about this fix.

dongjoon-hyun · 2018-11-10T00:08:50Z

Thanks. Technically, since it's categorized as a BUG, I'm +1 to have this in branch-2.4 as a syntax bug fix.

…eference ## What changes were proposed in this pull request? Very minor parser bug, but possibly problematic for code-generated queries: Consider the following two queries: ``` SELECT avg(k) OVER (w) FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1 ``` and ``` SELECT avg(k) OVER w FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1 ``` The former, with parens around the OVER condition, fails to parse while the latter, without parens, succeeds: ``` Error in SQL statement: ParseException: mismatched input '(' expecting {<EOF>, ',', 'FROM', 'WHERE', 'GROUP', 'ORDER', 'HAVING', 'LIMIT', 'LATERAL', 'WINDOW', 'UNION', 'EXCEPT', 'MINUS', 'INTERSECT', 'SORT', 'CLUSTER', 'DISTRIBUTE'}(line 1, pos 19) == SQL == SELECT avg(k) OVER (w) FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1 -------------------^^^ ``` This was found when running the cockroach DB tests. I tried PostgreSQL, The SQL with parentheses is also workable. ## How was this patch tested? Unit test Closes apache#22987 from gengliangwang/windowParentheses. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com>

…eference ## What changes were proposed in this pull request? Very minor parser bug, but possibly problematic for code-generated queries: Consider the following two queries: ``` SELECT avg(k) OVER (w) FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1 ``` and ``` SELECT avg(k) OVER w FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1 ``` The former, with parens around the OVER condition, fails to parse while the latter, without parens, succeeds: ``` Error in SQL statement: ParseException: mismatched input '(' expecting {<EOF>, ',', 'FROM', 'WHERE', 'GROUP', 'ORDER', 'HAVING', 'LIMIT', 'LATERAL', 'WINDOW', 'UNION', 'EXCEPT', 'MINUS', 'INTERSECT', 'SORT', 'CLUSTER', 'DISTRIBUTE'}(line 1, pos 19) == SQL == SELECT avg(k) OVER (w) FROM kv WINDOW w AS (PARTITION BY v ORDER BY w) ORDER BY 1 -------------------^^^ ``` This was found when running the cockroach DB tests. I tried PostgreSQL, The SQL with parentheses is also workable. ## How was this patch tested? Unit test Closes apache#22987 from gengliangwang/windowParentheses. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com> (cherry picked from commit 1db7997) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

fix

c9d01df

gengliangwang commented Nov 8, 2018

View reviewed changes

use window.sql for testing

2da6f99

cloud-fan reviewed Nov 9, 2018

View reviewed changes

cloud-fan approved these changes Nov 9, 2018

View reviewed changes

add newline

471092d

asfgit closed this in 1db7997 Nov 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-25979][SQL] Window function: allow parentheses around window reference #22987

[SPARK-25979][SQL] Window function: allow parentheses around window reference #22987

gengliangwang commented Nov 8, 2018

gengliangwang Nov 8, 2018

cloud-fan Nov 8, 2018

cloud-fan Nov 8, 2018

gengliangwang commented Nov 8, 2018

SparkQA commented Nov 8, 2018

cloud-fan Nov 9, 2018

gatorsmile Nov 9, 2018

gatorsmile commented Nov 9, 2018

SparkQA commented Nov 9, 2018

SparkQA commented Nov 9, 2018

gatorsmile commented Nov 9, 2018

dongjoon-hyun commented Nov 10, 2018

		@@ -31,6 +32,19 @@ class SQLWindowFunctionSuite extends QueryTest with SharedSQLContext {

		import testImplicits._

		val empSalaryData = Seq(

[SPARK-25979][SQL] Window function: allow parentheses around window reference #22987

[SPARK-25979][SQL] Window function: allow parentheses around window reference #22987

Conversation

gengliangwang commented Nov 8, 2018

What changes were proposed in this pull request?

How was this patch tested?

gengliangwang Nov 8, 2018

Choose a reason for hiding this comment

cloud-fan Nov 8, 2018

Choose a reason for hiding this comment

cloud-fan Nov 8, 2018

Choose a reason for hiding this comment

gengliangwang commented Nov 8, 2018

SparkQA commented Nov 8, 2018

cloud-fan Nov 9, 2018

Choose a reason for hiding this comment

gatorsmile Nov 9, 2018

Choose a reason for hiding this comment

gatorsmile commented Nov 9, 2018

SparkQA commented Nov 9, 2018

SparkQA commented Nov 9, 2018

gatorsmile commented Nov 9, 2018

dongjoon-hyun commented Nov 10, 2018