Skip to content

Conversation

@hvanhovell
Copy link
Contributor

This PR moves all the functionality provided by the SparkSQLParser/ExtendedHiveQlParser to the new Parser hierarchy (SparkQl/HiveQl). This also improves the current SET command parsing: the current implementation swallows set role ... and set autocommit ... commands, this PR respects these commands (and passes them on to Hive).

This PR and #10723 end the use of Parser-Combinator parsers for SQL parsing. As a result we can also remove the AbstractSQLParser in Catalyst.

The PR is marked WIP as long as it doesn't pass all tests.

cc @rxin @viirya @winningsix (this touches #10144)

@rxin
Copy link
Contributor

rxin commented Jan 26, 2016

"The PR is marked WIP as long as it doesn't pass all tests."

This is obvious so if you are only looking to run through the tests, we can just remove the WIP now. Then we don't need an extra roundtrip before merging.

@SparkQA
Copy link

SparkQA commented Jan 26, 2016

Test build #50039 has finished for PR 10905 at commit 45d537c.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

if you are updating the pr, can you add explicit types for all the public vals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed TAB's...

@SparkQA
Copy link

SparkQA commented Jan 27, 2016

Test build #50194 has finished for PR 10905 at commit 5177926.

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

@hvanhovell hvanhovell changed the title [SPARK-12865][SPARK-12866][SQL] Migrate SparkSQLParser/ExtendedHiveQlParser commands to new Parser [WIP] [SPARK-12865][SPARK-12866][SQL] Migrate SparkSQLParser/ExtendedHiveQlParser commands to new Parser Jan 27, 2016
Copy link
Contributor

Choose a reason for hiding this comment

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

would be great to briefly mention what this is used for (set?)

@rxin
Copy link
Contributor

rxin commented Jan 27, 2016

I'm going to merge this. Please address the minor feedback in a follow-up pr.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants