[SPARK-31975][SQL] Show AnalysisException when WindowFunction is used without WindowExpression #28808

ulysses-you · 2020-06-12T06:59:51Z

What changes were proposed in this pull request?

Add WindowFunction check at CheckAnalysis.

Why are the changes needed?

Provide friendly error msg.

BEFORE

scala> sql("select rank() from values(1)").show
java.lang.UnsupportedOperationException: Cannot generate code for expression: rank()

AFTER

scala> sql("select rank() from values(1)").show
org.apache.spark.sql.AnalysisException: Window function rank() requires an OVER clause.;;
Project [rank() AS RANK()#3]
+- LocalRelation [col1#2]

Does this PR introduce any user-facing change?

Yes, user wiill be given a better error msg.

How was this patch tested?

Pass the newly added UT.

SparkQA · 2020-06-12T07:36:42Z

Test build #123895 has finished for PR 28808 at commit f283385.

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

wangyum · 2020-06-12T08:42:26Z

retest this please.

SparkQA · 2020-06-12T09:18:21Z

Test build #123913 has finished for PR 28808 at commit f283385.

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

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

wangyum · 2020-06-12T15:15:37Z

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

@@ -158,6 +158,9 @@ trait CheckAnalysis extends PredicateHelper {
          case g: GroupingID =>
            failAnalysis("grouping_id() can only be used with GroupingSets/Cube/Rollup")

+          case Alias(w: WindowFunction, _) =>
+            failAnalysis(s"Window function '$w' call requires an OVER clause.")


Sorry, seems we need to remove single quotation marks:

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

Line 2734 in b2011a2

failAnalysis(s"Window function $wf requires window to be ordered, please add ORDER BY " +

SparkQA · 2020-06-12T16:28:18Z

Test build #123921 has finished for PR 28808 at commit 38df40a.

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

SparkQA · 2020-06-12T19:37:47Z

Test build #123932 has finished for PR 28808 at commit cd8bbb2.

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

SparkQA · 2020-06-13T07:00:24Z

Test build #123951 has finished for PR 28808 at commit 335935f.

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

wangyum · 2020-06-19T06:32:57Z

cc @jiangxb1987

ulysses-you · 2020-06-29T13:42:32Z

also cc @HyukjinKwon @cloud-fan a minor change.

cloud-fan · 2020-07-02T13:37:51Z

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

@@ -158,6 +158,9 @@ trait CheckAnalysis extends PredicateHelper {
          case g: GroupingID =>
            failAnalysis("grouping_id() can only be used with GroupingSets/Cube/Rollup")

+          case Alias(w: WindowFunction, _) =>
+            failAnalysis(s"Window function $w call requires an OVER clause.")


nit: we can remove call

cloud-fan

LGTM, please resolve the conflicts.

…1975

cloud-fan · 2020-07-02T14:47:55Z

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

@@ -158,6 +158,9 @@ trait CheckAnalysis extends PredicateHelper {
          case g: GroupingID =>
            failAnalysis("grouping_id() can only be used with GroupingSets/Cube/Rollup")

+          case Alias(w: WindowFunction, _) =>


BTW why do we have to match the Alias?

The right way is Alias(WindowExpression(w: WindowFunction, _), _) and without over it is Alias(w: WindowFunction, _).

then the check doesn't work for cases like rank() + 1?

good catch ! we need to check all expression.

SparkQA · 2020-07-02T19:26:51Z

Test build #124911 has finished for PR 28808 at commit 972ae13.

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

cloud-fan · 2020-07-03T11:40:55Z

retest this please

HyukjinKwon · 2020-07-03T12:11:35Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala

@@ -884,4 +884,15 @@ class AnalysisSuite extends AnalysisTest with Matchers {
      Seq("Intersect can only be performed on tables with the compatible column types. " +
        "timestamp <> double at the second column of the second table"))
  }
+
+  test("throw user facing error when use WindowFunction directly") {


Shall we add a JIRA prefix here? SPARK-31975: ...

maropu · 2020-07-03T22:35:57Z

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

@@ -158,6 +158,11 @@ trait CheckAnalysis extends PredicateHelper {
          case g: GroupingID =>
            failAnalysis("grouping_id() can only be used with GroupingSets/Cube/Rollup")

+          case e: Expression if e.children.exists(_.isInstanceOf[WindowFunction]) &&
+            !e.isInstanceOf[WindowExpression] =>


nit: indent

SparkQA · 2020-07-05T19:57:11Z

Test build #124949 has started for PR 28808 at commit 3e116f5.

dongjoon-hyun

+1, LGTM. Thank you, @ulysses-you .

BTW, @ulysses-you . I updated the PR title and the description a little.

HyukjinKwon · 2020-07-06T04:33:44Z

retest this please

ulysses-you · 2020-07-06T05:59:58Z

Thank you @dongjoon-hyun , thanks for help test @HyukjinKwon .

SparkQA · 2020-07-06T07:05:04Z

Test build #125024 has finished for PR 28808 at commit 3e116f5.

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

cloud-fan · 2020-07-06T10:36:38Z

retest this please

SparkQA · 2020-07-06T12:28:27Z

Test build #125064 has finished for PR 28808 at commit 3e116f5.

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

cloud-fan · 2020-07-06T12:36:36Z

retest this please

SparkQA · 2020-07-06T13:02:57Z

Test build #125082 has finished for PR 28808 at commit 3e116f5.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-07T04:02:37Z

retest this please

SparkQA · 2020-07-07T07:05:02Z

Test build #125175 has finished for PR 28808 at commit 3e116f5.

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

wangyum · 2020-07-07T07:15:12Z

retest this please.

SparkQA · 2020-07-07T13:26:33Z

Test build #125185 has finished for PR 28808 at commit 3e116f5.

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

cloud-fan · 2020-07-07T13:39:00Z

thanks, merging to master!

ulysses-you · 2020-07-07T23:57:12Z

thanks for merging! thanks all!

init

f283385

probot-autolabeler bot added the SQL label Jun 12, 2020

typo

38df40a

wangyum reviewed Jun 12, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala Outdated Show resolved Hide resolved

update msg

cd8bbb2

wangyum reviewed Jun 12, 2020

View reviewed changes

typo

335935f

cloud-fan reviewed Jul 2, 2020

View reviewed changes

cloud-fan approved these changes Jul 2, 2020

View reviewed changes

ulysses-you added 2 commits July 2, 2020 22:04

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

458d457

…1975

nit

972ae13

cloud-fan reviewed Jul 2, 2020

View reviewed changes

check all expression

0798fca

cloud-fan approved these changes Jul 3, 2020

View reviewed changes

HyukjinKwon reviewed Jul 3, 2020

View reviewed changes

HyukjinKwon approved these changes Jul 3, 2020

View reviewed changes

maropu reviewed Jul 3, 2020

View reviewed changes

maropu approved these changes Jul 3, 2020

View reviewed changes

nit

3e116f5

dongjoon-hyun changed the title ~~[SPARK-31975][SQL] Throw user facing error when use WindowFunction directly~~ [SPARK-31975][SQL] Show AnalysisException when WindowFunction is used without WindowExpression Jul 6, 2020

dongjoon-hyun approved these changes Jul 6, 2020

View reviewed changes

cloud-fan closed this in 2e23da2 Jul 7, 2020

ulysses-you deleted the SPARK-31975 branch March 11, 2021 10:14

[SPARK-31975][SQL] Show AnalysisException when WindowFunction is used without WindowExpression #28808

[SPARK-31975][SQL] Show AnalysisException when WindowFunction is used without WindowExpression #28808

Conversation

ulysses-you commented Jun 12, 2020 • edited by dongjoon-hyun Loading

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 Jun 12, 2020

wangyum commented Jun 12, 2020

SparkQA commented Jun 12, 2020

Choose a reason for hiding this comment

SparkQA commented Jun 12, 2020

SparkQA commented Jun 12, 2020

SparkQA commented Jun 13, 2020

wangyum commented Jun 19, 2020

ulysses-you commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan left a 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 Jul 2, 2020

cloud-fan commented Jul 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 5, 2020

dongjoon-hyun left a comment

Choose a reason for hiding this comment

HyukjinKwon commented Jul 6, 2020

ulysses-you commented Jul 6, 2020

SparkQA commented Jul 6, 2020

cloud-fan commented Jul 6, 2020

SparkQA commented Jul 6, 2020

cloud-fan commented Jul 6, 2020

SparkQA commented Jul 6, 2020

HyukjinKwon commented Jul 7, 2020

SparkQA commented Jul 7, 2020

wangyum commented Jul 7, 2020

SparkQA commented Jul 7, 2020

cloud-fan commented Jul 7, 2020

ulysses-you commented Jul 7, 2020

ulysses-you commented Jun 12, 2020 •

edited by dongjoon-hyun

Loading