Skip to content

Conversation

@wangyum
Copy link
Member

@wangyum wangyum commented Jun 12, 2019

What changes were proposed in this pull request?

This PR is to port date.sql from PostgreSQL regression tests. https://github.com/postgres/postgres/blob/REL_12_BETA2/src/test/regress/sql/date.sql

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

When porting the test cases, found four PostgreSQL specific features that do not exist in Spark SQL:

SPARK-28017: Enhance EXTRACT/DATE_TRUNC
SPARK-28141: Date type can not accept special values
SPARK-28253: Date type have different low value and high value
SPARK-28259: Date/Time Output Styles and Date Order Conventions

Also, found a bug:
SPARK-28015: Invalid date formats should throw an exception

Also, found a inconsistent behavior:
SPARK-27923: Invalid date throw an exception bug Spark SQL returns NULL, for example: https://github.com/postgres/postgres/blob/30bcebbdcf23eb8b78e553c4b3b5eb847410ef19/src/test/regress/expected/date.out#L13-L14

How was this patch tested?

N/A

@SparkQA
Copy link

SparkQA commented Jun 12, 2019

Test build #106413 has finished for PR 24850 at commit 69470df.

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

--
--
-- DATE
-- https://github.com/postgres/postgres/blob/REL_12_BETA1/src/test/regress/sql/date.sql
Copy link
Member

Choose a reason for hiding this comment

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

PostgresSQL 12 Beta2 was released on 20th June.

Could you check https://github.com/postgres/postgres/blob/REL_12_BETA1/src/test/regress/sql/date.sql and update this PR (if needed)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only these files changed:
image

Copy link
Member

@dongjoon-hyun dongjoon-hyun Jul 4, 2019

Choose a reason for hiding this comment

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

Thanks. Then, let's update this line 6 from REL_12_BETA1 to REL_12_BETA2 because we had better point to the latest branch.

Copy link
Member

Choose a reason for hiding this comment

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

btw, we don't need to have the same snapshot of these migrated pg tests? In sql-tests/inputs/pgSQL, already-merged tests comes from REL_12_BETA1 and the other tests will come from REL_12_BETA2?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I think so, @maropu . If we started with REL_11, it might be a different story. However, we don't need to stick to BETA1. It's unofficially one. We just picked up them because that was the latest one at that time.

Also, we can update them later consistently and easily. For now, it seems that the best effort is to keep using the latest one at the merge time.

Copy link
Member

Choose a reason for hiding this comment

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

ok, thanks!

@wangyum wangyum changed the title [WIP][SPARK-28020][SQL][TEST] Port date.sql [SPARK-28020][SQL][TEST] Port date.sql Jul 4, 2019
@SparkQA
Copy link

SparkQA commented Jul 5, 2019

Test build #107245 has finished for PR 24850 at commit 06f8bf1.

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

@dongjoon-hyun
Copy link
Member

Only a few comments. Also, please read and update the PR description (e.g. it still mentions BETA1).

@SparkQA
Copy link

SparkQA commented Jul 5, 2019

Test build #107282 has finished for PR 24850 at commit 589745d.

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

@SparkQA
Copy link

SparkQA commented Jul 6, 2019

Test build #107293 has finished for PR 24850 at commit 52ccc4f.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. This is the current status of master branch.
Merged to master.
Thank you, @wangyum and @maropu .

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.

4 participants