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.

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

[SPARK-14591] [SQL] Remove DataTypeParser and add more keywords to the nonReserved list. #12796

Closed
wants to merge 3 commits into from
Closed

Conversation

yhuai
Copy link
Contributor

@yhuai yhuai commented Apr 30, 2016

What changes were proposed in this pull request?

CatalystSqlParser can parse data types. So, we do not need to have an individual DataTypeParser.

How was this patch tested?

Existing tests

@SparkQA
Copy link

SparkQA commented Apr 30, 2016

Test build #57388 has finished for PR 12796 at commit 8a13ffc.

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

@@ -674,6 +674,8 @@ nonReserved
| AT | NULLS | OVERWRITE | ALL | ALTER | AS | BETWEEN | BY | CREATE | DELETE
| DESCRIBE | DROP | EXISTS | FALSE | FOR | GROUP | IN | INSERT | INTO | IS |LIKE
| NULL | ORDER | OUTER | TABLE | TRUE | WITH | RLIKE
| AND | CASE | CAST | CROSS | DISTINCT | DIV | ELSE | END | FROM | FUNCTION | HAVING | INTERVAL | JOIN|
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @hvanhovell

Does adding things to this list cause any problems (performance?)?

@@ -674,6 +674,8 @@ nonReserved
| AT | NULLS | OVERWRITE | ALL | ALTER | AS | BETWEEN | BY | CREATE | DELETE
| DESCRIBE | DROP | EXISTS | FALSE | FOR | GROUP | IN | INSERT | INTO | IS |LIKE
| NULL | ORDER | OUTER | TABLE | TRUE | WITH | RLIKE
| AND | CASE | CAST | CROSS | DISTINCT | DIV | ELSE | END | FROM | FUNCTION | HAVING | INTERVAL | JOIN
| MACRO | NOT | ON | OR | SELECT | STRATIFY | THEN | UNBOUNDED | WHEN | WHERE
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From @rxin

cc @hvanhovell
Does adding things to this list cause any problems (performance?)?

@SparkQA
Copy link

SparkQA commented Apr 30, 2016

Test build #57392 has finished for PR 12796 at commit 31a5267.

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

@yhuai
Copy link
Contributor Author

yhuai commented Apr 30, 2016

I have reverted the parser change. Since CatalystSqlParser always has reserved keywords, let's decide if we want to expand the list of non reserved keywords in future.

It's still good to only have a single parser to avoid any potential confusion.

@SparkQA
Copy link

SparkQA commented Apr 30, 2016

Test build #57406 has finished for PR 12796 at commit fdba779.

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

@rxin
Copy link
Contributor

rxin commented Apr 30, 2016

Thanks - merging in master.

@asfgit asfgit closed this in ac41fc6 Apr 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants