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-13139][SQL] Follow-ups to #11573 #11667

Closed
wants to merge 2 commits into from

Conversation

andrewor14
Copy link
Contributor

What changes were proposed in this pull request?

Addressing outstanding comments in #11573.

How was this patch tested?

Jenkins, new test case in DDLCommandSuite

@andrewor14
Copy link
Contributor Author

@yhuai

@SparkQA
Copy link

SparkQA commented Mar 12, 2016

Test build #52967 has finished for PR 11667 at commit 8ed596d.

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

@hvanhovell
Copy link
Contributor

LGTM

@@ -36,6 +36,18 @@ private[sql] class SparkQl(conf: ParserConf = SimpleParserConf()) extends Cataly
/**
* For each node, extract properties in the form of a list ['key1', 'key2', 'key3', 'value']
* into a pair (key1.key2.key3, value).
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Not very related to this PR. @hvanhovell Do you know when we will have something like

TOK_TABLEPROPERTY
  'key_part1'
  'key_part2'
  'key_part3'
  'value'

(the form that matches the original comment extract properties in the form of a list ['key1', 'key2', 'key3', 'value'] into a pair (key1.key2.key3, value).)

Copy link
Contributor

Choose a reason for hiding this comment

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

That happens when we have a dot-separated key, e.g.: a.b.c The parser identifies each part of the identifier separately, e.g: a, b & c. It is a bit of a PITA to re-construct this in ANTLR3, so we currently do this in the tree parser.

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewor14 I will change For each node, extract properties in the form of a list ['key1', 'key2', 'key3', 'value'] into a pair (key1.key2.key3, value). to For each node, extract properties in the form of a list ['key_part1', 'key_part2', 'key_part3', 'value'] into a pair (key_part1.key_part2.key_part3, value).. Hopefully, this will make the comment easier to understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hvanhovell Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sg

@yhuai
Copy link
Contributor

yhuai commented Mar 14, 2016

LGTM. I am merging this to master.

@asfgit asfgit closed this in 9a1680c Mar 14, 2016
@andrewor14 andrewor14 deleted the ddl-parser-followups branch March 14, 2016 17:46
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Mar 17, 2016
Addressing outstanding comments in apache#11573.

Jenkins, new test case in `DDLCommandSuite`

Author: Andrew Or <andrew@databricks.com>

Closes apache#11667 from andrewor14/ddl-parser-followups.
roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016
Addressing outstanding comments in apache#11573.

Jenkins, new test case in `DDLCommandSuite`

Author: Andrew Or <andrew@databricks.com>

Closes apache#11667 from andrewor14/ddl-parser-followups.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants