[SPARK-22360][SQL][TEST] Add unit tests for Window Specifications #20045

smurakozi · 2017-12-21T14:25:51Z

What changes were proposed in this pull request?

Improve the test coverage of window specifications.

New tests cover basic cases more systematically in DataFrameWindowFunctionsSuite:

different partition clauses (none, one, multiple)
different order clauses (none, one, multiple, asc/desc, nulls first/last)
defaults if clauses are missing

New tests were added to cover some more complex cases when partitionBy or orderBy uses expressions.

ExpressionParserSuite.'window function expressions' was also extended to check parsing of some additional window expressions.

How was this patch tested?

Only new tests were added, automated tests were executed.

…sions

gaborgsomogyi · 2017-12-21T14:41:09Z

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

+        'v,
+        lead("v", 1).over(Window.orderBy($"k1".desc, $"k2")),
+        lead("v", 1).over(Window.orderBy($"k1", $"k2".desc)),
+        lead("v", 1).over(Window.orderBy($"k1".desc, $"k2"))),


As I see multiple orderBy but what is the benefit to see the exact same calculation in the 2nd and 4th column?

It should be desc, desc, thanks for catching it.

gaborgsomogyi · 2017-12-21T15:02:47Z

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

+    assertEqual("foo(*) over (order by a desc, b asc)", windowed(Seq.empty, Seq('a.desc, 'b.asc)))
+    assertEqual("foo(*) over (sort by a)", windowed(Seq.empty, Seq('a.asc)))
+    assertEqual("foo(*) over (sort by a desc, b asc)", windowed(Seq.empty, Seq('a.desc, 'b.asc)))
    assertEqual("foo(*) over (partition by a, b order by c)", windowed(Seq('a, 'b), Seq('c.asc)))


(partition by a order by c) is a possibility in order to be conform with the pattern you followed.

I will fix it, thx

gaborgsomogyi · 2017-12-21T15:03:25Z

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

+    assertEqual("foo(*) over (sort by a)", windowed(Seq.empty, Seq('a.asc)))
+    assertEqual("foo(*) over (sort by a desc, b asc)", windowed(Seq.empty, Seq('a.desc, 'b.asc)))
    assertEqual("foo(*) over (partition by a, b order by c)", windowed(Seq('a, 'b), Seq('c.asc)))
    assertEqual("foo(*) over (distribute by a, b sort by c)", windowed(Seq('a, 'b), Seq('c.asc)))


(distribute by a order by c) is a possibility in order to be conform with the pattern you followed.

I will fix it, thx

squito · 2017-12-21T19:07:48Z

Jenkins, ok to test

SparkQA · 2017-12-21T21:40:31Z

Test build #85277 has finished for PR 20045 at commit 797c907.

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

squito · 2017-12-21T22:07:37Z

failure seems to be unrelated ...

Jenkins, retest this please

attilapiros · 2017-12-21T15:10:42Z

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

+  }
+
+
+  test("Order by without frame defaults to range between unbounded_preceding - current_row") {


I would propose using an easier aggregate function to follow this behaviour: collect_list.
And using one more projection where "rangeBetween(Window.unboundedPreceding, Window.currentRow)" is explicitly given. Moreover desc direction can be tested too.

Like:

test("Order by without frame defaults to range between unbounded_preceding - current_row") { val df = Seq( ("a", "p1", "1"), ("b", "p1", "2"), ("c", "p1", "2")).toDF("key", "partition", "value") checkAnswer( df.select( $"key", collect_list("value").over(Window.partitionBy($"partition").orderBy($"value")), collect_list("value").over(Window.partitionBy($"partition").orderBy($"value") .rangeBetween(Window.unboundedPreceding, Window.currentRow)), collect_list("value").over(Window.partitionBy($"partition").orderBy($"value".desc)), collect_list("value").over(Window.partitionBy($"partition").orderBy($"value".desc) .rangeBetween(Window.unboundedPreceding, Window.currentRow))), Seq( Row("a", Array("1"), Array("1"), Array("2", "2", "1"), Array("2", "2", "1")), Row("b", Array("1", "2", "2"), Array("1", "2", "2"), Array("2", "2"), Array("2", "2")), Row("c", Array("1", "2", "2"), Array("1", "2", "2"), Array("2", "2"), Array("2", "2")))) }

SparkQA · 2017-12-22T23:07:37Z

Test build #4023 has finished for PR 20045 at commit 797c907.

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

SparkQA · 2018-01-02T23:44:17Z

Test build #85612 has finished for PR 20045 at commit a0100d1.

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

smurakozi · 2018-01-03T15:13:52Z

@gatorsmile @hvanhovell @jiangxb1987, could you have a look, please?

SparkQA · 2018-01-17T09:14:48Z

Test build #86256 has finished for PR 20045 at commit 28e1738.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-17T09:24:57Z

Test build #86258 has finished for PR 20045 at commit a3d3429.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-17T15:55:58Z

Test build #86267 has finished for PR 20045 at commit 04d9e0f.

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

smurakozi · 2018-01-17T20:22:21Z

@gatorsmile @hvanhovell @jiangxb1987, could you have a look, please?

jiangxb1987 · 2018-01-17T21:19:15Z

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

    // Basic window testing.
    assertEqual("foo(*) over w1", UnresolvedWindowExpression(func, WindowSpecReference("w1")))
    assertEqual("foo(*) over ()", windowed())
+    assertEqual("foo(*) over (partition by a)", windowed(Seq('a)))


You shall also cover the null cases, such as foo(*) over (partition by null)

SparkQA · 2018-01-22T16:37:39Z

Test build #86475 has finished for PR 20045 at commit 5feb4f7.

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

smurakozi · 2018-01-29T13:20:37Z

Do you think I need to cover any other cases, @jiangxb1987 ?

smurakozi · 2018-03-05T08:46:59Z

I think I've addressed all your points @jiangxb1987.
Do you think I need any more changes?

jiangxb1987 · 2018-03-05T09:11:10Z

Sorry for the delay, I'll check the results later this week.

smurakozi · 2018-03-14T12:49:41Z

ping @jiangxb1987

smurakozi · 2018-04-09T14:34:19Z

ping @jiangxb1987

rxin · 2018-04-09T16:34:36Z

Can we add them to the file based test suites instead?

HyukjinKwon · 2018-07-16T02:22:19Z

ok to test

HyukjinKwon · 2018-07-16T02:22:30Z

gentle ping @smurakozi

SparkQA · 2018-07-16T06:32:17Z

Test build #93049 has finished for PR 20045 at commit 5feb4f7.

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

dongjoon-hyun · 2019-06-15T04:38:16Z

Do you still need this, @jiangxb1987 ? Also, gentle ping, @smurakozi .

gaborgsomogyi · 2019-06-17T08:22:34Z

I'm pretty sure @smurakozi will not finish this PR.

HyukjinKwon · 2019-06-17T08:45:30Z

Closing due to inactivity here.

smurakozi added 3 commits December 21, 2017 12:26

New test cases for window specification

313aafb

test for filtering based on window function

6c48095

More test expressions in ExpressionParserSuite.window function expres…

e941aca

…sions

gaborgsomogyi reviewed Dec 21, 2017

View reviewed changes

Fixes based on review comments

797c907

attilapiros reviewed Dec 22, 2017

View reviewed changes

smurakozi changed the title ~~[Spark-22360][SQL] Add unit tests for Window Specifications~~ [Spark-22360][SQL][TEST] Add unit tests for Window Specifications Dec 23, 2017

Using rank instead of lead in orderBy tests for better readability.

a0100d1

Merge branch 'master' into SPARK-22360

28e1738

Fix issue caused by merge

a3d3429

Fixed a scala style issue

04d9e0f

jiangxb1987 reviewed Jan 17, 2018

View reviewed changes

Added null cases to basic window tests

5feb4f7

dongjoon-hyun changed the title ~~[Spark-22360][SQL][TEST] Add unit tests for Window Specifications~~ [SPARK-22360][SQL][TEST] Add unit tests for Window Specifications Jun 15, 2019

dongjoon-hyun added SQL TESTS labels Jun 15, 2019

HyukjinKwon closed this Jun 17, 2019

		}


		test("Order by without frame defaults to range between unbounded_preceding - current_row") {

[SPARK-22360][SQL][TEST] Add unit tests for Window Specifications #20045

[SPARK-22360][SQL][TEST] Add unit tests for Window Specifications #20045

Uh oh!

Conversation

smurakozi commented Dec 21, 2017

What changes were proposed in this pull request?

How was this patch tested?

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squito commented Dec 21, 2017

Uh oh!

SparkQA commented Dec 21, 2017

Uh oh!

squito commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 22, 2017

Uh oh!

SparkQA commented Jan 2, 2018

Uh oh!

smurakozi commented Jan 3, 2018

Uh oh!

SparkQA commented Jan 17, 2018

Uh oh!

SparkQA commented Jan 17, 2018

Uh oh!

SparkQA commented Jan 17, 2018

Uh oh!

smurakozi commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 22, 2018

Uh oh!

smurakozi commented Jan 29, 2018

Uh oh!

smurakozi commented Mar 5, 2018

Uh oh!

jiangxb1987 commented Mar 5, 2018

Uh oh!

smurakozi commented Mar 14, 2018

Uh oh!

smurakozi commented Apr 9, 2018

Uh oh!

rxin commented Apr 9, 2018

Uh oh!

HyukjinKwon commented Jul 16, 2018

Uh oh!

HyukjinKwon commented Jul 16, 2018

Uh oh!

SparkQA commented Jul 16, 2018

Uh oh!

dongjoon-hyun commented Jun 15, 2019

Uh oh!

gaborgsomogyi commented Jun 17, 2019

Uh oh!

HyukjinKwon commented Jun 17, 2019

Uh oh!

Uh oh!