[SPARK-29108][SQL][TESTS][FOLLOWUP] Comment out no use test case and add 'insert into' statement of window.sql (Part 2) #27439

beliefer · 2020-02-03T02:13:14Z

What changes were proposed in this pull request?

When I running the window_part2.sql tests find it lack insert sql. Therefore, the output is empty.
I checked the postgresql and reference https://github.com/postgres/postgres/blob/master/src/test/regress/sql/window.sql
Although window_part1.sql and window_part3.sql exists the insert sql, I think should also add it into window_part2.sql.
Because only one case reference the table empsalary and it throws AnalysisException.

-- !query
select last(salary) over(order by salary range between 1000 preceding and 1000 following),
lag(salary) over(order by salary range between 1000 preceding and 1000 following),
salary from empsalary
-- !query schema
struct<>
-- !query output
org.apache.spark.sql.AnalysisException
Window Frame specifiedwindowframe(RangeFrame, -1000, 1000) must match the required frame specifiedwindowframe(RowFrame, -1, -1);

So we should do four work:

comment out the only one case and create a new ticket.
Add INSERT INTO empsalary.

Note: window_part4.sql not use the table empsalary.

Why are the changes needed?

Supplementary test data.

Does this PR introduce any user-facing change?

No

How was this patch tested?

New test case

SparkQA · 2020-02-03T06:08:28Z

Test build #117756 has finished for PR 27439 at commit b792307.

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

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/window_part2.sql

beliefer · 2020-02-04T11:52:32Z

cc @dongjoon-hyun @maropu

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/window_part2.sql

HyukjinKwon · 2020-02-05T03:32:20Z

BTW, @beliefer, don't forget to update PR description and title accordingly.

beliefer · 2020-02-05T03:46:34Z

BTW, @beliefer, don't forget to update PR description and title accordingly.

Thanks for your reminder.

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

Test build #117876 has finished for PR 27439 at commit 50d713e.

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

maropu · 2020-02-05T07:45:54Z

retest this please

SparkQA · 2020-02-05T08:05:02Z

Test build #117908 has finished for PR 27439 at commit 5929571.

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

SparkQA · 2020-02-05T08:05:02Z

Test build #117883 has finished for PR 27439 at commit 5929571.

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

beliefer · 2020-02-05T08:29:57Z

retest this please

SparkQA · 2020-02-05T13:05:40Z

Test build #117921 has finished for PR 27439 at commit 5929571.

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

beliefer · 2020-02-05T13:30:31Z

retest this please

dongjoon-hyun

Sorry, but I'm -1 for this change, @beliefer , @HyukjinKwon , and @HyukjinKwon .
If we start to support more queries in this file, we need to bring them back.
Let's not decide what is not required at this stage.

SparkQA · 2020-02-05T18:19:01Z

Test build #117935 has finished for PR 27439 at commit 5929571.

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

sql/core/src/test/resources/sql-tests/results/postgreSQL/window_part2.sql.out

HyukjinKwon · 2020-02-06T00:58:31Z

If we start to support more queries in this file, we need to bring them back.
Let's not decide what is not required at this stage.

@dongjoon-hyun, the flip side also works. For the time we don't support, we can save one INSERT and CREATE operation in test base until that point. I don't think keeping unused data or codes in test cases is right. We should bring it back only when it's used. (e.g., we should remove unused codes rather then keeping it just in case it's used in the future).

Just to be clear, is this inconsistent with other ported tests in PostgreSQL? If that's the case, let's just revert back to revive CREATE and INSERT - I don't mind just keeping it simple and consistent.; otherwise, I think we can just go ahead.

dongjoon-hyun · 2020-02-06T01:05:21Z

@HyukjinKwon .
The same author (@beliefer ) are actively working this area.

NTH: [SPARK-27951][SQL] Support ANSI SQL NTH_VALUE window function #27440
FIRST_VALUE/LAST_VALUE: he wrote on the JIRA, he will start after NTH.

dongjoon-hyun · 2020-02-06T01:07:04Z

I'm expecting more PRs on master. However, for branch-3.0, I agree with you. You can optimize that from branch-3.0 because we will not backport new feature~

HyukjinKwon · 2020-02-06T01:08:22Z

Thanks @dongjoon-hyun. @beliefer, do you plan to support these soon? If that's the case let's bring CREATE and INSERT back here.

beliefer · 2020-02-06T01:19:09Z

Thanks @dongjoon-hyun. @beliefer, do you plan to support these soon? If that's the case let's bring CREATE and INSERT back here.

I think some feature will be supported, the other ones will not.

HyukjinKwon · 2020-02-06T01:35:26Z

If you plan to create a PRs to support them in this file, let's just don't comment CREATE and INSERT out here.

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/window_part2.sql

HyukjinKwon · 2020-02-06T02:01:00Z

LGTM otherwise. Sorry for a bit of back and forth @beliefer but mind updating PR description back as well?

beliefer · 2020-02-06T02:32:28Z

LGTM otherwise. Sorry for a bit of back and forth @beliefer but mind updating PR description back as well?

OK.

SparkQA · 2020-02-06T06:20:29Z

Test build #117959 has finished for PR 27439 at commit 4e27e98.

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

HyukjinKwon · 2020-02-06T06:23:54Z

Merged to master, and branch-3.0 to match it with JIRA's

…add 'insert into' statement of window.sql (Part 2) ### What changes were proposed in this pull request? When I running the `window_part2.sql` tests find it lack insert sql. Therefore, the output is empty. I checked the postgresql and reference https://github.com/postgres/postgres/blob/master/src/test/regress/sql/window.sql Although `window_part1.sql` and `window_part3.sql` exists the insert sql, I think should also add it into `window_part2.sql`. Because only one case reference the table `empsalary` and it throws `AnalysisException`. ``` -- !query select last(salary) over(order by salary range between 1000 preceding and 1000 following), lag(salary) over(order by salary range between 1000 preceding and 1000 following), salary from empsalary -- !query schema struct<> -- !query output org.apache.spark.sql.AnalysisException Window Frame specifiedwindowframe(RangeFrame, -1000, 1000) must match the required frame specifiedwindowframe(RowFrame, -1, -1); ``` So we should do four work: 1. comment out the only one case and create a new ticket. 2. Add `INSERT INTO empsalary`. Note: window_part4.sql not use the table `empsalary`. ### Why are the changes needed? Supplementary test data. ### Does this PR introduce any user-facing change? No ### How was this patch tested? New test case Closes #27439 from beliefer/add-insert-to-window. Authored-by: beliefer <beliefer@163.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>

beliefer · 2020-02-06T06:41:15Z

@HyukjinKwon @dongjoon-hyun Thanks for your work.

SparkQA · 2020-02-06T07:27:22Z

Test build #117963 has finished for PR 27439 at commit 025c328.

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

…add 'insert into' statement of window.sql (Part 2) ### What changes were proposed in this pull request? When I running the `window_part2.sql` tests find it lack insert sql. Therefore, the output is empty. I checked the postgresql and reference https://github.com/postgres/postgres/blob/master/src/test/regress/sql/window.sql Although `window_part1.sql` and `window_part3.sql` exists the insert sql, I think should also add it into `window_part2.sql`. Because only one case reference the table `empsalary` and it throws `AnalysisException`. ``` -- !query select last(salary) over(order by salary range between 1000 preceding and 1000 following), lag(salary) over(order by salary range between 1000 preceding and 1000 following), salary from empsalary -- !query schema struct<> -- !query output org.apache.spark.sql.AnalysisException Window Frame specifiedwindowframe(RangeFrame, -1000, 1000) must match the required frame specifiedwindowframe(RowFrame, -1, -1); ``` So we should do four work: 1. comment out the only one case and create a new ticket. 2. Add `INSERT INTO empsalary`. Note: window_part4.sql not use the table `empsalary`. ### Why are the changes needed? Supplementary test data. ### Does this PR introduce any user-facing change? No ### How was this patch tested? New test case Closes apache#27439 from beliefer/add-insert-to-window. Authored-by: beliefer <beliefer@163.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>

add insert sql

b792307

HyukjinKwon reviewed Feb 4, 2020

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/window_part2.sql Show resolved Hide resolved

HyukjinKwon reviewed Feb 5, 2020

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/window_part2.sql Show resolved Hide resolved

Update comment

50d713e

HyukjinKwon approved these changes Feb 5, 2020

View reviewed changes

HyukjinKwon reviewed Feb 5, 2020

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/window_part2.sql Outdated Show resolved Hide resolved

beliefer changed the title ~~[SPARK-29108][SQL][TESTS][FOLLOWUP] Add insert sql to window.sql (Part 2)~~ [SPARK-29108][SQL][TESTS][FOLLOWUP] Update coment to window.sql (Part 2) Feb 5, 2020

beliefer changed the title ~~[SPARK-29108][SQL][TESTS][FOLLOWUP] Update coment to window.sql (Part 2)~~ [SPARK-29108][SQL][TESTS][FOLLOWUP] Comment out no use and insert into statement of window.sql (Part 2) Feb 5, 2020

Update comment

5929571

maropu approved these changes Feb 5, 2020

View reviewed changes

dongjoon-hyun requested changes Feb 5, 2020

View reviewed changes

dongjoon-hyun added SQL TESTS labels Feb 5, 2020

dongjoon-hyun reviewed Feb 5, 2020

View reviewed changes

sql/core/src/test/resources/sql-tests/results/postgreSQL/window_part2.sql.out Show resolved Hide resolved

Revert some test cases.

4e27e98

HyukjinKwon reviewed Feb 6, 2020

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/window_part2.sql Outdated Show resolved Hide resolved

comment out the only test case not supported.

025c328

beliefer changed the title ~~[SPARK-29108][SQL][TESTS][FOLLOWUP] Comment out no use and insert into statement of window.sql (Part 2)~~ [SPARK-29108][SQL][TESTS][FOLLOWUP] Comment out no use test case and add 'insert into' statement of window.sql (Part 2) Feb 6, 2020

HyukjinKwon closed this in c8ef1de Feb 6, 2020

beliefer deleted the add-insert-to-window branch May 12, 2020 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-29108][SQL][TESTS][FOLLOWUP] Comment out no use test case and add 'insert into' statement of window.sql (Part 2) #27439

[SPARK-29108][SQL][TESTS][FOLLOWUP] Comment out no use test case and add 'insert into' statement of window.sql (Part 2) #27439

beliefer commented Feb 3, 2020 •

edited

Loading

SparkQA commented Feb 3, 2020

beliefer commented Feb 4, 2020

HyukjinKwon commented Feb 5, 2020

beliefer commented Feb 5, 2020

SparkQA commented Feb 5, 2020

maropu commented Feb 5, 2020

SparkQA commented Feb 5, 2020

SparkQA commented Feb 5, 2020

beliefer commented Feb 5, 2020

SparkQA commented Feb 5, 2020

beliefer commented Feb 5, 2020

dongjoon-hyun left a comment •

edited

Loading

SparkQA commented Feb 5, 2020

HyukjinKwon commented Feb 6, 2020 •

edited

Loading

dongjoon-hyun commented Feb 6, 2020

dongjoon-hyun commented Feb 6, 2020

HyukjinKwon commented Feb 6, 2020

beliefer commented Feb 6, 2020

HyukjinKwon commented Feb 6, 2020

HyukjinKwon commented Feb 6, 2020

beliefer commented Feb 6, 2020

SparkQA commented Feb 6, 2020

HyukjinKwon commented Feb 6, 2020 •

edited

Loading

beliefer commented Feb 6, 2020

SparkQA commented Feb 6, 2020

[SPARK-29108][SQL][TESTS][FOLLOWUP] Comment out no use test case and add 'insert into' statement of window.sql (Part 2) #27439

[SPARK-29108][SQL][TESTS][FOLLOWUP] Comment out no use test case and add 'insert into' statement of window.sql (Part 2) #27439

Conversation

beliefer commented Feb 3, 2020 • edited 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 Feb 3, 2020

beliefer commented Feb 4, 2020

HyukjinKwon commented Feb 5, 2020

beliefer commented Feb 5, 2020

SparkQA commented Feb 5, 2020

maropu commented Feb 5, 2020

SparkQA commented Feb 5, 2020

SparkQA commented Feb 5, 2020

beliefer commented Feb 5, 2020

SparkQA commented Feb 5, 2020

beliefer commented Feb 5, 2020

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

SparkQA commented Feb 5, 2020

HyukjinKwon commented Feb 6, 2020 • edited Loading

dongjoon-hyun commented Feb 6, 2020

dongjoon-hyun commented Feb 6, 2020

HyukjinKwon commented Feb 6, 2020

beliefer commented Feb 6, 2020

HyukjinKwon commented Feb 6, 2020

HyukjinKwon commented Feb 6, 2020

beliefer commented Feb 6, 2020

SparkQA commented Feb 6, 2020

HyukjinKwon commented Feb 6, 2020 • edited Loading

beliefer commented Feb 6, 2020

SparkQA commented Feb 6, 2020

beliefer commented Feb 3, 2020 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

HyukjinKwon commented Feb 6, 2020 •

edited

Loading

HyukjinKwon commented Feb 6, 2020 •

edited

Loading