Skip to content

Conversation

@DylanGuedes
Copy link
Contributor

@DylanGuedes DylanGuedes commented Oct 15, 2019

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_STABLE/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_STABLE/src/test/regress/expected/window.out

Why are the changes needed?

To ensure compatibility with PostgreSQL.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass the Jenkins. And, Comparison with PgSQL results.

Signed-off-by: DylanGuedes <djmgguedes@gmail.com>
@dongjoon-hyun
Copy link
Member

ok to test

@dongjoon-hyun
Copy link
Member

Thank you, @DylanGuedes !

@dongjoon-hyun
Copy link
Member

cc @gengliangwang

@SparkQA
Copy link

SparkQA commented Oct 15, 2019

Test build #112081 has finished for PR 26119 at commit 0128b5d.

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

@gengliangwang
Copy link
Member

@DylanGuedes Thanks for the work.
Please wait until #26107 is merged.

@dongjoon-hyun
Copy link
Member

#26107 is merged. Thanks, @gengliangwang .

@dongjoon-hyun
Copy link
Member

Retest this please.

@SparkQA
Copy link

SparkQA commented Oct 15, 2019

Test build #112123 has finished for PR 26119 at commit 0128b5d.

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

@dongjoon-hyun
Copy link
Member

According to the failures, we need to update the PR.

- Cannot safely cast 'enroll_date': StringType to DateType;

@DylanGuedes
Copy link
Contributor Author

@dongjoon-hyun should I JIRA that?

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

SparkQA commented Oct 16, 2019

Test build #112130 has finished for PR 26119 at commit 2c8d3ef.

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

@HyukjinKwon
Copy link
Member

@DylanGuedes, please skip or fix the tests being failed in the thrift server. cc @wangyum

@DylanGuedes
Copy link
Contributor Author

@HyukjinKwon I have no idea why they are breaking on Thrift, they used to pass in the older PR and they also were fine in my local config (I can't rerun then now because my environment suddenly broken). Do you have any suggestion? I don't think that makes sense to just comment out anything related to date since date.sql passed fine and I also don't know why this kind of test is failing on Thrift.

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

SparkQA commented Oct 22, 2019

Test build #112420 has finished for PR 26119 at commit 6cd07c8.

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

@HyukjinKwon
Copy link
Member

You can either skip the single test with a comment or add it in a blacklist I think cc @wangyum
I would file a separate jira for that.

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

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. I will merge in few days.

@SparkQA
Copy link

SparkQA commented Oct 22, 2019

Test build #112422 has finished for PR 26119 at commit 2448dfc.

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

@HyukjinKwon
Copy link
Member

Merged to master.

@maropu
Copy link
Member

maropu commented Oct 23, 2019

Thanks for the work, @DylanGuedes! btw, how many PR parts are left for window.sql? It seems we have no open PRs for this.

@DylanGuedes
Copy link
Contributor Author

@maropu Two parts are missing. I thought that there were more people interested on working on them because I read some comments on JIRA, but since no one opened a MR, I'll create a new one for the other parts and work them as well.

@maropu
Copy link
Member

maropu commented Oct 24, 2019

Thanks, again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants