[SPARK-22361][SQL][TEST] Add unit test for Window Frames #20019

gaborgsomogyi · 2017-12-19T13:28:14Z

What changes were proposed in this pull request?

There are already quite a few integration tests using window frames, but the unit tests coverage is not ideal.

In this PR the already existing tests are reorganized, extended and where gaps found additional cases added.

How was this patch tested?

Automated: Pass the Jenkins.

gaborgsomogyi · 2017-12-19T13:32:17Z

cc @jiangxb1987 @gatorsmile @hvanhovell

gatorsmile · 2017-12-20T04:50:40Z

ok to test

SparkQA · 2017-12-20T07:29:47Z

Test build #85153 has finished for PR 20019 at commit 87e61ce.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DataFrameWindowFramesSuite extends QueryTest with SharedSQLContext

smurakozi

Add a test checking that rangeBetween throws an AnalysisException if it gets a non-numeric column as input, as demonstrated by
Seq("a").toDF("k").select(min('k).over(Window.orderBy('k).rangeBetween(-1, 1))).show

Add a test checking that rangeBetween throws an AnalysisException if it gets a column ordered by multiple order by expressions, as demonstrated by
Seq((1, 2)).toDF("k1", "k2").select(min('k1).over(Window.orderBy('k1, 'k2).rangeBetween(-1, 1))).show

Add tests checking that the above mentioned rules are ignored if range is not bounded, as demonstrated by
Seq((1, 2)).toDF("k1", "k2").select(min('k1).over(Window.orderBy('k1, 'k2).rangeBetween(Window.unboundedPreceding, Window.unboundedFollowing))).show

and
Seq("a").toDF("k").select(min('k).over(Window.orderBy('k).rangeBetween(Window.unboundedPreceding, Window.unboundedFollowing))).show

gaborgsomogyi · 2017-12-20T13:57:42Z

@smurakozi nice catch, added them. Additionally found a nit.

SparkQA · 2017-12-20T16:18:55Z

Test build #85190 has finished for PR 20019 at commit a2fa042.

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

jiangxb1987

Thanks for the effort! Only some minor issues.
It would also be great to improve the test for window frames parser, which currently is in ExpressionParserSuite.test("window function expressions").

jiangxb1987 · 2017-12-21T05:43:42Z

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

+    assert(e.message.contains("Boundary end is not a valid integer: 2147483648"))
+  }
+
+  test("range between should accept at most one ORDER BY expression when unbounded") {


"unbounded" -> "bounded"

jiangxb1987 · 2017-12-21T05:46:23Z

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

+      "window specification with multiple order by expressions"))
+  }
+
+  test("range between should accept numeric values only when bounded") {


This is actually a TypeCoercion issue, I'm not sure whether we should cover it here or TypeCoercionSuite

I bit enhanced this area but not yet moved. What do you think, would it be good idea to move this test there? This is as window related as type coercion.

jiangxb1987 · 2017-12-21T05:57:45Z

also cc @gatorsmile whom I believe should be interested to look at this.

gaborgsomogyi · 2017-12-21T14:28:55Z

ExpressionParserSuite window related part is a bit reorganized and enhanced.

SparkQA · 2017-12-21T17:15:57Z

Test build #85267 has finished for PR 20019 at commit fa20e73.

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

jiangxb1987 · 2017-12-22T16:06:12Z

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

@@ -263,21 +263,60 @@ class ExpressionParserSuite extends PlanTest {
      "sum(product + 1) over (partition by ((product / 2) + 1) order by 2)",
      WindowExpression('sum.function('product + 1),
        WindowSpecDefinition(Seq('product / 2 + 1), Seq(Literal(2).asc), UnspecifiedFrame)))
+  }
+
+  test("range/rows window function expressions") {


How about window function expressions on various window frames?

Just back from holiday. What do you mean exactly here? Range/rows between test with most of the combinations are covered. If you could give an example it would be helpful.

@jiangxb1987 gentle ping

sorry for the late response. Just update the test case name.

jiangxb1987 · 2017-12-22T16:10:52Z

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

    val frameTypes = Seq(("rows", RowFrame), ("range", RangeFrame))
    val boundaries = Seq(
-      ("10 preceding", -Literal(10), CurrentRow),
+      ("unbounded preceding", UnboundedPreceding, CurrentRow),


tiny nit: could you add comments on each group of boundaries to explain what they are trying to cover?

Change is on the way...

SparkQA · 2018-01-02T12:48:51Z

Test build #85592 has finished for PR 20019 at commit 677a4d6.

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

SparkQA · 2018-01-08T19:34:20Z

Test build #85803 has finished for PR 20019 at commit d1e2454.

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

jiangxb1987 · 2018-01-16T19:25:17Z

retest this please

SparkQA · 2018-01-16T22:39:04Z

Test build #86191 has finished for PR 20019 at commit d1e2454.

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

jiangxb1987 · 2018-01-16T22:52:02Z

LGTM

gatorsmile · 2018-01-17T02:02:19Z

Thanks! Merged to master/2.3

## What changes were proposed in this pull request? There are already quite a few integration tests using window frames, but the unit tests coverage is not ideal. In this PR the already existing tests are reorganized, extended and where gaps found additional cases added. ## How was this patch tested? Automated: Pass the Jenkins. Author: Gabor Somogyi <[email protected]> Closes #20019 from gaborgsomogyi/SPARK-22361. (cherry picked from commit a9b845e) Signed-off-by: gatorsmile <[email protected]>

gaborgsomogyi · 2018-01-17T13:28:19Z

@gatorsmile @jiangxb1987 @smurakozi Thanks for the help!

Add unit test for Window Frames

87e61ce

smurakozi suggested changes Dec 20, 2017

View reviewed changes

Review fix

a2fa042

jiangxb1987 reviewed Dec 21, 2017

View reviewed changes

ExpressionParserSuite window related test enhanced + review fixes

fa20e73

jiangxb1987 reviewed Dec 22, 2017

View reviewed changes

Added group comment for boundaries

677a4d6

gaborgsomogyi added 2 commits January 8, 2018 17:00

Merge branch 'master' into SPARK-22361

6dfc77e

Test fix

d1e2454

asfgit closed this in a9b845e Jan 17, 2018

gaborgsomogyi deleted the SPARK-22361 branch January 17, 2018 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-22361][SQL][TEST] Add unit test for Window Frames #20019

[SPARK-22361][SQL][TEST] Add unit test for Window Frames #20019

gaborgsomogyi commented Dec 19, 2017

gaborgsomogyi commented Dec 19, 2017

gatorsmile commented Dec 20, 2017

SparkQA commented Dec 20, 2017

smurakozi left a comment

gaborgsomogyi commented Dec 20, 2017

SparkQA commented Dec 20, 2017

jiangxb1987 left a comment

jiangxb1987 Dec 21, 2017

gaborgsomogyi Dec 21, 2017

jiangxb1987 Dec 21, 2017

gaborgsomogyi Dec 21, 2017

jiangxb1987 commented Dec 21, 2017

gaborgsomogyi commented Dec 21, 2017

SparkQA commented Dec 21, 2017

jiangxb1987 Dec 22, 2017

gaborgsomogyi Jan 2, 2018 •

edited

Loading

gaborgsomogyi Jan 16, 2018

jiangxb1987 Jan 16, 2018

jiangxb1987 Dec 22, 2017

gaborgsomogyi Jan 2, 2018

SparkQA commented Jan 2, 2018

SparkQA commented Jan 8, 2018

jiangxb1987 commented Jan 16, 2018

SparkQA commented Jan 16, 2018

jiangxb1987 commented Jan 16, 2018

gatorsmile commented Jan 17, 2018

gaborgsomogyi commented Jan 17, 2018

[SPARK-22361][SQL][TEST] Add unit test for Window Frames #20019

[SPARK-22361][SQL][TEST] Add unit test for Window Frames #20019

Conversation

gaborgsomogyi commented Dec 19, 2017

What changes were proposed in this pull request?

How was this patch tested?

gaborgsomogyi commented Dec 19, 2017

gatorsmile commented Dec 20, 2017

SparkQA commented Dec 20, 2017

smurakozi left a comment

Choose a reason for hiding this comment

gaborgsomogyi commented Dec 20, 2017

SparkQA commented Dec 20, 2017

jiangxb1987 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

jiangxb1987 commented Dec 21, 2017

gaborgsomogyi commented Dec 21, 2017

SparkQA commented Dec 21, 2017

Choose a reason for hiding this comment

gaborgsomogyi Jan 2, 2018 • edited Loading

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 Jan 2, 2018

SparkQA commented Jan 8, 2018

jiangxb1987 commented Jan 16, 2018

SparkQA commented Jan 16, 2018

jiangxb1987 commented Jan 16, 2018

gatorsmile commented Jan 17, 2018

gaborgsomogyi commented Jan 17, 2018

gaborgsomogyi Jan 2, 2018 •

edited

Loading