Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "[SPARK-23264][SQL] Make INTERVAL keyword optional when ANSI enabled" #27080

Closed
wants to merge 3 commits into from
Closed

Revert "[SPARK-23264][SQL] Make INTERVAL keyword optional when ANSI enabled" #27080

wants to merge 3 commits into from

Conversation

cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented Jan 2, 2020 •
edited
Loading

What changes were proposed in this pull request?

Revert #20433 .

Why are the changes needed?

According to the SQL standard, the INTERVAL prefix is required:

<interval literal> ::=
  INTERVAL [ <sign> ] <interval string> <interval qualifier>

<interval string> ::=
  <quote> <unquoted interval string> <quote>

Does this PR introduce any user-facing change?

yes, but omitting the INTERVAL prefix is a new feature in 3.0

How was this patch tested?

existing tests

cloud-fan
cloud-fan commented Jan 2, 2020
View reviewed changes
docs/sql-keywords.md
@@ -87,8 +87,7 @@ Below is a list of all the keywords in Spark SQL.
<tr><td>DATA</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
<tr><td>DATABASE</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
<tr><td>DATABASES</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
<tr><td>DAY</td><td>reserved</td><td>non-reserved</td><td>reserved</td></tr>
<tr><td>DAYS</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to https://www.postgresql.org/docs/current/sql-keywords-appendix.html , unit-S is not keyword.

cloud-fan
cloud-fan commented Jan 2, 2020
View reviewed changes
docs/sql-keywords.md
<tr><td>MICROSECOND</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
<tr><td>MICROSECONDS</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
<tr><td>MILLISECOND</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
<tr><td>MILLISECONDS</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to https://www.postgresql.org/docs/current/sql-keywords-appendix.html , they are not keyword.

cloud-fan
cloud-fan commented Jan 2, 2020
View reviewed changes
docs/sql-keywords.md
@@ -293,12 +284,9 @@ Below is a list of all the keywords in Spark SQL.
<tr><td>USING</td><td>reserved</td><td>strict-non-reserved</td><td>reserved</td></tr>
<tr><td>VALUES</td><td>non-reserved</td><td>non-reserved</td><td>reserved</td></tr>
<tr><td>VIEW</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
<tr><td>WEEK</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to https://www.postgresql.org/docs/current/sql-keywords-appendix.html , this is not keyword.

@cloud-fan
Copy link
Contributor Author

cc @maropu @gatorsmile

@SparkQA
Copy link

SparkQA commented Jan 2, 2020

Test build #116048 has finished for PR 27080 at commit 01d134b.

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

@SparkQA
Copy link

SparkQA commented Jan 2, 2020

Test build #116050 has finished for PR 27080 at commit ee86c8a.

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

maropu
maropu approved these changes Jan 2, 2020
View reviewed changes
Copy link
Member

@maropu maropu left a comment

Choose a reason for hiding this comment

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

Looks ok to me if the tests passed.

maropu
maropu reviewed Jan 2, 2020
View reviewed changes
sql/core/src/test/resources/sql-tests/inputs/ansi/interval.sql
--IMPORT interval.sql
Copy link
Member

Choose a reason for hiding this comment

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

Still need this test file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, the overflow behavior is different.

Copy link
Member

Choose a reason for hiding this comment

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

In general, we should have a comment to explain why we need this file.

HyukjinKwon
HyukjinKwon approved these changes Jan 3, 2020
View reviewed changes
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 if tests pass

@HyukjinKwon HyukjinKwon changed the title revert [SPARK-23264][SQL] Make INTERVAL keyword optional when ANSI enabled Revert "[SPARK-23264][SQL] Make INTERVAL keyword optional when ANSI enabled" Jan 3, 2020
@yaooqinn
Copy link
Member

yaooqinn commented Jan 3, 2020

LGTM

@SparkQA
Copy link

SparkQA commented Jan 3, 2020

Test build #116064 has finished for PR 27080 at commit 87289cb.

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

@SparkQA
Copy link

SparkQA commented Jan 3, 2020

Test build #116077 has finished for PR 27080 at commit d22635f.

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

@cloud-fan
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Jan 3, 2020

Test build #116083 has finished for PR 27080 at commit 305b9e3.

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

@SparkQA
Copy link

SparkQA commented Jan 3, 2020

Test build #116095 has finished for PR 27080 at commit 91a0e33.

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

@gatorsmile gatorsmile closed this in be4faaf Jan 3, 2020
@gatorsmile
Copy link
Member

Thanks! Merged to master.

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

@maropu maropu maropu approved these changes

@gatorsmile gatorsmile gatorsmile left review comments

@viirya viirya viirya approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

Assignees
No one assigned
Labels
SQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cloud-fan @SparkQA @yaooqinn @gatorsmile @viirya @maropu @HyukjinKwon @dongjoon-hyun