Skip to content

[SPARK-12689][SQL] Migrate DDL parsing to the newly absorbed parser #10723

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

Closed
viirya wants to merge 20 commits into from
Closed

[SPARK-12689][SQL] Migrate DDL parsing to the newly absorbed parser #10723

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f886f67
Migrate DDL parsing to the newly absorbed parser: describe command.
viirya Jan 12, 2016
c60cd9e
Migrate refreshTable command.
viirya Jan 13, 2016
f2d6fa6
Migrate createTable ddl command.
viirya Jan 18, 2016
5a6cc4a
Merge remote-tracking branch 'upstream/master' into migrate-ddl-describe
viirya Jan 18, 2016
78f1b7c
Remove quotes from option values.
viirya Jan 18, 2016
1c145eb
Allow dotted provider name.
viirya Jan 18, 2016
d800c58
Address comments and fix bug.
viirya Jan 19, 2016
838f701
Remove DDLParser.
viirya Jan 19, 2016
7e0f218
Merge remote-tracking branch 'upstream/master' into migrate-ddl-describe
viirya Jan 19, 2016
559083e
Fix MiMa test.
viirya Jan 19, 2016
17ce6ae
Merge remote-tracking branch 'upstream/master' into migrate-ddl-describe
viirya Jan 21, 2016
4fc7a60
Fix test by allowing using DOT in option key and support comment in c…
viirya Jan 21, 2016
3e5a229
Fix test.
viirya Jan 21, 2016
9922ccc
Fix another problem.
viirya Jan 21, 2016
cbd3173
Fix test.
viirya Jan 22, 2016
1df2b80
For comments.
viirya Jan 25, 2016
9ff32e0
For comment.
viirya Jan 25, 2016
7350f07
Merge remote-tracking branch 'upstream/master' into migrate-ddl-describe
viirya Jan 28, 2016
7d31844
For comments.
viirya Jan 28, 2016
8b7086e
Merge remote-tracking branch 'upstream/master' into migrate-ddl-describe
viirya Jan 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions sql/catalyst/src/main/antlr3/org/apache/spark/sql/catalyst/parser/SparkSqlLexer.g
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,8 @@ KW_ISOLATION: 'ISOLATION';
KW_LEVEL: 'LEVEL';
KW_SNAPSHOT: 'SNAPSHOT';
KW_AUTOCOMMIT: 'AUTOCOMMIT';
KW_REFRESH: 'REFRESH';


// Operators
// NOTE: if you add a new function/operator, add it to sysFuncNames so that describe function _FUNC_ will work.
Expand Down
9 changes: 9 additions & 0 deletions sql/catalyst/src/main/antlr3/org/apache/spark/sql/catalyst/parser/SparkSqlParser.g
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ TOK_TXN_READ_WRITE;
TOK_COMMIT;
TOK_ROLLBACK;
TOK_SET_AUTOCOMMIT;
TOK_REFRESHTABLE;
}


Expand Down Expand Up @@ -759,6 +760,7 @@ ddlStatement
| truncateTableStatement
| alterStatement
| descStatement
| refreshStatement
| showStatement
| metastoreCheck
| createViewStatement
Expand Down Expand Up @@ -1357,6 +1359,13 @@ tabPartColTypeExpr
: tableName partitionSpec? extColumnName? -> ^(TOK_TABTYPE tableName partitionSpec? extColumnName?)
;

refreshStatement
@init { pushMsg("refresh statement", state); }
@after { popMsg(state); }
:
KW_REFRESH KW_TABLE tableName -> ^(TOK_REFRESHTABLE tableName)
;

descStatement
@init { pushMsg("describe statement", state); }
@after { popMsg(state); }
Expand Down
21 changes: 15 additions & 6 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/SparkQl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import org.apache.spark.sql.catalyst.{CatalystQl, TableIdentifier}
import org.apache.spark.sql.catalyst.analysis.UnresolvedRelation
import org.apache.spark.sql.catalyst.parser.{ASTNode, ParserConf, SimpleParserConf}
import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, OneRowRelation}
import org.apache.spark.sql.execution.datasources.RefreshTable

private[sql] class SparkQl(conf: ParserConf = SimpleParserConf()) extends CatalystQl(conf) {
/** Check if a command should not be explained. */
Expand All @@ -42,6 +43,10 @@ private[sql] class SparkQl(conf: ParserConf = SimpleParserConf()) extends Cataly
getClauses(Seq("TOK_QUERY", "FORMATTED", "EXTENDED"), explainArgs)
ExplainCommand(nodeToPlan(query), extended = extended.isDefined)

case Token("TOK_REFRESHTABLE", nameParts :: Nil) =>
val tableIdent = extractTableIdent(nameParts)
RefreshTable(tableIdent)

case Token("TOK_DESCTABLE", describeArgs) =>
// Reference: https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL
val Some(tableType) :: formatted :: extended :: pretty :: Nil =
Expand All @@ -52,26 +57,30 @@ private[sql] class SparkQl(conf: ParserConf = SimpleParserConf()) extends Cataly
nodeToDescribeFallback(node)
} else {
tableType match {
case Token("TOK_TABTYPE", Token("TOK_TABNAME", nameParts :: Nil) :: Nil) =>
case Token("TOK_TABTYPE", Token("TOK_TABNAME", nameParts) :: Nil) =>

@cloud-fan cloud-fan Jan 13, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why change this? You didn't touch the describe stuff in SparkSqlParser.g right?

@viirya viirya Jan 13, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. I think it is incorrect from beginning but not be tested it out because we don't reach here before. I've tested it locally. Once all three commands are migrated, we can see this passing tests.

@hvanhovell hvanhovell Jan 18, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we parse the following SQL using the parse driver org.apache.spark.sql.catalyst.parser.ParseDriver.parsePlan("DESCRIBE EXTENDED tbl.a", null)

We would end up with the following AST:

TOK_DESCTABLE 1, 0, 6, 18
:- TOK_TABTYPE 1, 4, 6, 18 
:  +- TOK_TABNAME 1, 4, 6, 18 
:     :- tbl 1, 4, 4, 18 
:     +- a 1, 6, 6, 22 
+- EXTENDED 1, 2, 2, 9

This change would pick this up, and old code didn't (I am sure I tested this though :S ). You can disable this in the DDL parser, to see if it works now.

@hvanhovell hvanhovell Jan 23, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we add a test for this? The Hive test suite apparently misses this one. I could also address in a different PR.

@viirya viirya Jan 26, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually we have test for describe table command in HiveQuerySuite. Do we need another test?

nameParts match {
case Token(".", dbName :: tableName :: Nil) =>
case Token(dbName, _) :: Token(tableName, _) :: Nil =>
// It is describing a table with the format like "describe db.table".
// TODO: Actually, a user may mean tableName.columnName. Need to resolve this
// issue.
val tableIdent = extractTableIdent(nameParts)
val tableIdent = TableIdentifier(
cleanIdentifier(tableName), Some(cleanIdentifier(dbName)))
datasources.DescribeCommand(
UnresolvedRelation(tableIdent, None), isExtended = extended.isDefined)
case Token(".", dbName :: tableName :: colName :: Nil) =>
case Token(dbName, _) :: Token(tableName, _) :: Token(colName, _) :: Nil =>
// It is describing a column with the format like "describe db.table column".
nodeToDescribeFallback(node)
case tableName =>
case tableName :: Nil =>
// It is describing a table with the format like "describe table".
datasources.DescribeCommand(
UnresolvedRelation(TableIdentifier(tableName.text), None),

@hvanhovell hvanhovell Jan 18, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cleanIdentifier?

isExtended = extended.isDefined)
case _ =>
nodeToDescribeFallback(node)
}
// All other cases.
case _ => nodeToDescribeFallback(node)
case _ =>
nodeToDescribeFallback(node)
}
}

Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DDLParser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class DDLParser(parseQuery: String => LogicalPlan)
protected val COMMENT = Keyword("COMMENT")
protected val REFRESH = Keyword("REFRESH")

protected lazy val ddl: Parser[LogicalPlan] = createTable | describeTable | refreshTable
protected lazy val ddl: Parser[LogicalPlan] = createTable

protected def start: Parser[LogicalPlan] = ddl

Expand Down