[SPARK-14348][SQL] Support native execution of SHOW TBLPROPERTIES command #12133
Conversation
| def isTemporaryTable(name: TableIdentifier): Boolean = { | ||
| val db = name.database.getOrElse(currentDb) | ||
| val table = formatTableName(name.table) | ||
| if (!name.database.isDefined && tempTables.contains(table)) { |
There was a problem hiding this comment.
yeah, please do !name.database.isDefined && tempTables.contains(table)
There was a problem hiding this comment.
Thanks.. will make the change.
|
Test build #54783 has finished for PR 12133 at commit
|
|
Test build #54789 has finished for PR 12133 at commit
|
| if (catalog.isTemporaryTable(table)) { | ||
| throw new AnalysisException("This operation is unsupported for temporary tables") | ||
| } | ||
| val catalogTable = sqlContext.sessionState.catalog.getTable(table) |
There was a problem hiding this comment.
I'd use tableExists to check if the table exists. If not, throw an analysis exception.
There was a problem hiding this comment.
Simon, you mean we should call a tableExist first before calling the getTable ? getTable does return a TableNotFound exception already if table is not found..
There was a problem hiding this comment.
I am not sure if we can expect all external catalogs have this behavior. Because the return type of getTable is CatalogTable, instead of Option[CatalogTable], I'd expect it might return a null. At least I didn't see this in getTable's comment. If it is true, I think it is better to say it.
There was a problem hiding this comment.
Its documented here
Its good to also add it in the SessionCatalog.getTable doc. I will add a comment.
There was a problem hiding this comment.
Yeah. Thanks. I think it is better.
|
Test build #54798 has finished for PR 12133 at commit
|
| } | ||
| } | ||
|
|
||
| test("show tables") { |
There was a problem hiding this comment.
Why is this test here? You are testing SHOW TABLES commands, instead of SHOW TBLPROPERTIES commands.
|
Test build #54805 has finished for PR 12133 at commit
|
| propertyKey: Option[String]) extends RunnableCommand { | ||
|
|
||
| override val output: Seq[Attribute] = { | ||
| val withKeySchema: Seq[Attribute] = { |
There was a problem hiding this comment.
MINOR/NIT: This is still more elaborate than it needs to be. Why not:
val schema = AttributeReference("value", StringType, nullable = false)() :: Nil
propertyKey match {
case None => AttributeReference("key", StringType, nullable = false)() :: schema
case _ => schema
}
There was a problem hiding this comment.
Thanks !! It looks much better now !! I have made the change ..
|
Test build #54808 has finished for PR 12133 at commit
|
|
cc @hvanhovell |
|
LGTM |
|
Merging to master. Thanks! |
What changes were proposed in this pull request?
This PR adds Native execution of SHOW TBLPROPERTIES command.
Command Syntax:
How was this patch tested?
Tests added in HiveComandSuiie and DDLCommandSuite