[SPARK-29107][SQL][TESTS] Port window.sql (Part 1) #25816

DylanGuedes · 2019-09-17T10:23:43Z

What changes were proposed in this pull request?

This PR ports window.sql from PostgreSQL regression tests https://github.com/postgres/postgres/blob/REL_12_BETA2/src/test/regress/sql/window.sql from lines 1~319

The expected results can be found in the link: https://github.com/postgres/postgres/blob/REL_12_BETA2/src/test/regress/expected/window.out

How was this patch tested?

Pass the Jenkins.

Why are the changes needed?

To ensure compatibility with PGSQL

Does this PR introduce any user-facing change?

No

How was this patch tested?

Comparison with PgSQL results.

wangyum · 2019-09-17T13:41:09Z

ok to test

SparkQA · 2019-09-17T13:48:49Z

Test build #110785 has started for PR 25816 at commit a8a3695.

shaneknapp · 2019-09-17T17:40:38Z

test this please

SparkQA · 2019-09-17T23:33:47Z

Test build #110801 has finished for PR 25816 at commit a8a3695.

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

maropu

LGTM except for some minor comments. @wangyum @HyukjinKwon

maropu · 2019-09-19T04:23:42Z

sql/core/src/test/resources/sql-tests/inputs/pgSQL/window_part1.sql

+-- Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+--
+-- Window Functions Testing
+-- https://github.com/postgres/postgres/blob/REL_12_BETA3/src/test/regress/sql/window.sql


Can you add line numbers?:

spark/sql/core/src/test/resources/sql-tests/inputs/pgSQL/aggregates_part1.sql

Line 6 in a6a663c

-- https://github.com/postgres/postgres/blob/REL_12_BETA2/src/test/regress/sql/aggregates.sql#L1-L143

Unfortunately,REL_12_BETA4 is released.

https://github.com/postgres/postgres/blob/REL_12_BETA4/src/test/regress/sql/window.sql

I can update to match Beta4, but:

If the PgSQL version is that relevant, then we will add a new JIRA for every new PgSQL release for every .sql migration (i.e: strings.sql, union.sql, date.sql, aggregates.sql, etc). But we didn't.

If the PgSQL is not that relevant, then there's no need to change PRs to match the latest release candidate version (although it matches a stable version).

What you think?

Have you checked the last discussion? #24850 (comment)
I think we don't need to stick to the single snapshot, so its ok to update it to REL_12_BETA4 in this port. Actually, no difference between BETA2 and BETA4 in window.sql?

No, I didn't get noticed that you guys were already discussing about updating other tests.
I checked last week and there are some major differences between BETA2 and BETA4 for window.sql, I just can't remember if lines L1-L319 are impacted by them. So, it's ok to keep at REL2 or should I updated to REL3?

maropu · 2019-09-19T04:42:53Z

sql/core/src/test/resources/sql-tests/inputs/pgSQL/window_part1.sql

+SELECT sum(salary) OVER w, rank() OVER w FROM empsalary WINDOW w AS (PARTITION BY depname ORDER BY salary DESC);
+
+-- strict aggs
+-- Temporarily turns off the ANSI mode because of compatibility issues between keywords


Can you describe which keyword?

maropu · 2019-09-19T04:49:30Z

sql/core/src/test/resources/sql-tests/inputs/pgSQL/window_part1.sql

+FROM tenk1 WHERE unique1 < 10;
+
+-- [SPARK-28428] Spark `exclude` always expecting `()`
+SELECT sum(unique1) over (w range between current row and unbounded following),


Why didn't you comment out this statement? You did the other statements, though?

sql/core/src/test/resources/sql-tests/inputs/pgSQL/window_part1.sql

HyukjinKwon · 2019-09-29T07:34:29Z

ping @DylanGuedes to update

DylanGuedes · 2019-09-29T11:16:03Z

@HyukjinKwon I updated with the changes suggested, but still not clear: should I update to REL4?

SparkQA · 2019-09-29T11:35:21Z

Test build #111564 has finished for PR 25816 at commit 6f08f75.

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

HyukjinKwon · 2019-09-29T12:37:32Z

Yea, we should update it.

SparkQA · 2019-09-29T13:34:42Z

Test build #111567 has finished for PR 25816 at commit ad93404.

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

HyukjinKwon · 2019-09-29T14:07:53Z

sql/core/src/test/resources/sql-tests/results/pgSQL/window_part1.sql.out

+3	9	25	9
+
+
+-- !query 43


This test seems failing when it's ran via the thrift server. Can you check the cause and exclude this in the test? cc @wangyum

SparkQA · 2019-09-29T18:31:00Z

Test build #111571 has finished for PR 25816 at commit 0783234.

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

dongjoon-hyun · 2019-09-29T21:44:45Z

sql/core/src/test/resources/sql-tests/inputs/pgSQL/window_part1.sql

+-- SELECT four, ten/4 as two,
+-- sum(ten/4) over (partition by four order by ten/4 range between unbounded preceding and current row),
+-- last(ten/4) over (partition by four order by ten/4 range between unbounded preceding and current row)
+-- FROM (select distinct ten, four from tenk1) ss;


Ur, did you regenerate the golden files after this removal?

SparkQA · 2019-09-29T22:17:50Z

Test build #111575 has finished for PR 25816 at commit b6fe45c.

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

SparkQA · 2019-10-02T01:26:20Z

Test build #111661 has finished for PR 25816 at commit 178bbfe.

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

dongjoon-hyun · 2019-10-03T04:01:24Z

Hmm. Still Result did not match for query for the following.

SELECT four, ten/4 as two,
sum(ten/4) over (partition by four order by ten/4 range between unbounded preceding and current row),
last(ten/4) over (partition by four order by ten/4 range between unbounded preceding and current row)
FROM (select distinct ten, four from tenk1) ss

@DylanGuedes . Could you post the query result in both spark-shell and spark-sql?
Previously, there was a bug which the result is different in both shells and spark-sql is similar with Spark Thrift Server.

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

DylanGuedes · 2019-10-08T01:38:03Z

It works fine locally. And, to be fair, I have no idea why this is not working: in my older PR It also always passed. I'll remote both queries that are failing.

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

SparkQA · 2019-10-08T05:32:04Z

Test build #111862 has finished for PR 25816 at commit 3a5825c.

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

dongjoon-hyun · 2019-10-13T00:48:13Z

Retest this please.

dongjoon-hyun · 2019-10-13T00:50:40Z

sql/core/src/test/resources/sql-tests/inputs/pgSQL/window_part1.sql

+last(ten) over (partition by four order by ten range between unbounded preceding and unbounded following)
+FROM (select distinct ten, four from tenk1) ss;
+
+-- Failing on thrift server


We need a JIRA issue if we are not going to cover this query.

dongjoon-hyun · 2019-10-13T00:51:04Z

sql/core/src/test/resources/sql-tests/inputs/pgSQL/window_part1.sql

+-- last(ten/4) over (partition by four order by ten/4 range between unbounded preceding and current row)
+-- FROM (select distinct ten, four from tenk1) ss;
+
+-- Failing on thrift server


We need a JIRA issue if we are not going to cover this query, too.

dongjoon-hyun

I know this consumed your time your time, @DylanGuedes .

I believe the followings are the final pieces.

A new JIRA for the newly excluded query 1: [SPARK-29107][SQL][TESTS] Port window.sql (Part 1) #25816 (comment)
A new JIRA for the newly excluded query 2: [SPARK-29107][SQL][TESTS] Port window.sql (Part 1) #25816 (comment)
Fix the PR inconsistency. (1) PR description has a link to REL_12_BETA2. (2) Code has a link to REL_12_BETA4. (3) The latest one is already REL_12_STABLE. The best option is to sync with REL_12_STABLE. Could you check the diff? I believe there is no diff on this file. Then, we can use REL_12_STABLE.

cc @maropu , too.

dongjoon-hyun · 2019-10-13T01:26:50Z

I checked the diff. There is additional addition from line 1257, but that is irrelevant to this PR because this PR is only for line 1 to line 319.

~/A/postgres:REL_12_STABLE$ git diff REL_12_BETA4 src/test/regress/sql/window.sql
diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql
index fc6d4cc903..fe273aa31e 100644
--- a/src/test/regress/sql/window.sql
+++ b/src/test/regress/sql/window.sql
@@ -1257,3 +1257,22 @@ SELECT to_char(SUM(n::float8) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND 1 FO
 SELECT i, b, bool_and(b) OVER w, bool_or(b) OVER w
   FROM (VALUES (1,true), (2,true), (3,false), (4,false), (5,true)) v(i,b)
   WINDOW w AS (ORDER BY i ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING);
+
+-- Tests for problems with failure to walk or mutate expressions
+-- within window frame clauses.
+
+-- test walker (fails with collation error if expressions are not walked)
+SELECT array_agg(i) OVER w
+  FROM generate_series(1,5) i
+WINDOW w AS (ORDER BY i ROWS BETWEEN (('foo' < 'foobar')::integer) PRECEDING AND CURRENT ROW);
+
+-- test mutator (fails when inlined if expressions are not mutated)
+CREATE FUNCTION pg_temp.f(group_size BIGINT) RETURNS SETOF integer[]
+AS $$
+    SELECT array_agg(s) OVER w
+      FROM generate_series(1,5) s
+    WINDOW w AS (ORDER BY s ROWS BETWEEN CURRENT ROW AND GROUP_SIZE FOLLOWING)
+$$ LANGUAGE SQL STABLE;
+
+EXPLAIN (costs off) SELECT * FROM pg_temp.f(2);
+SELECT * FROM pg_temp.f(2);

Please simply update the link with REL_12_STABLE in the PR description and code.

SparkQA · 2019-10-13T04:25:37Z

Test build #111988 has finished for PR 25816 at commit 3a5825c.

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

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

SparkQA · 2019-10-13T14:34:22Z

Test build #111995 has finished for PR 25816 at commit 1ac60f1.

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

dongjoon-hyun

+1, LGTM. Thank you, @DylanGuedes , @maropu , @HyukjinKwon !
Merged to master for this.

gengliangwang · 2019-10-14T21:21:16Z

I have created a followup to correct the file location: #26116

dongjoon-hyun · 2019-10-14T22:14:33Z

Oops. Yes. We moved the location and this PR is very old.
For now, #26116 is failing. I'll revert this PR. Sorry guys.

dongjoon-hyun · 2019-10-14T22:20:05Z

I missed the location, @DylanGuedes .
Sorry, but could you make a PR after putting the file into the correct location sql/core/src/test/resources/sql-tests/inputs/postgreSQL instead of pgSQL?
As @gengliangwang pointed out, this PR may fail at that new location. We can decide to file a JIRA or fix it at that time.

And, thank you again, @gengliangwang !

DylanGuedes · 2019-10-15T00:03:53Z

@dongjoon-hyun I made a new PR with the new location.

dongjoon-hyun added SQL TESTS labels Sep 18, 2019

maropu approved these changes Sep 19, 2019

View reviewed changes

HyukjinKwon reviewed Sep 29, 2019

View reviewed changes

dongjoon-hyun reviewed Sep 29, 2019

View reviewed changes

DylanGuedes added 6 commits October 7, 2019 22:32

adds first part of window split

df2a00a

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

add new changes to window_part1

e23bced

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

update to rel4

3d2c538

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

remove query that didnt pass on thrift

589ddf6

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

update golden file

2eafd2c

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

change queries

1062e9c

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

regen golden file

3a5825c

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

DylanGuedes force-pushed the spark-29107 branch from 178bbfe to 3a5825c Compare October 8, 2019 01:55

dongjoon-hyun reviewed Oct 13, 2019

View reviewed changes

dongjoon-hyun requested changes Oct 13, 2019

View reviewed changes

bump window file version and add new jiras

1ac60f1

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>

dongjoon-hyun approved these changes Oct 14, 2019

View reviewed changes

dongjoon-hyun closed this in 81915da Oct 14, 2019

gengliangwang mentioned this pull request Oct 14, 2019

[SPARK-29107][SQL][TESTS][FollowUp] Move window_part1.sql to directory 'postgreSQL' #26116

Closed

[SPARK-29107][SQL][TESTS] Port window.sql (Part 1) #25816

[SPARK-29107][SQL][TESTS] Port window.sql (Part 1) #25816

Conversation

DylanGuedes commented Sep 17, 2019

What changes were proposed in this pull request?

How was this patch tested?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

wangyum commented Sep 17, 2019

SparkQA commented Sep 17, 2019

shaneknapp commented Sep 17, 2019

SparkQA commented Sep 17, 2019

maropu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Sep 29, 2019

DylanGuedes commented Sep 29, 2019

SparkQA commented Sep 29, 2019

HyukjinKwon commented Sep 29, 2019

SparkQA commented Sep 29, 2019

Choose a reason for hiding this comment

SparkQA commented Sep 29, 2019

Choose a reason for hiding this comment

SparkQA commented Sep 29, 2019

SparkQA commented Oct 2, 2019

dongjoon-hyun commented Oct 3, 2019

DylanGuedes commented Oct 8, 2019

SparkQA commented Oct 8, 2019

dongjoon-hyun commented Oct 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Oct 13, 2019 • edited Loading

SparkQA commented Oct 13, 2019

SparkQA commented Oct 13, 2019

dongjoon-hyun left a comment

Choose a reason for hiding this comment

gengliangwang commented Oct 14, 2019

dongjoon-hyun commented Oct 14, 2019

dongjoon-hyun commented Oct 14, 2019

DylanGuedes commented Oct 15, 2019

dongjoon-hyun commented Oct 13, 2019 •

edited

Loading