[SPARK-16458][SQL] SessionCatalog should support `listColumns` for temporary tables #14114

dongjoon-hyun · 2016-07-09T10:01:14Z

What changes were proposed in this pull request?

Temporary tables are used frequently, but spark.catalog.listColumns does not support those tables. This PR make SessionCatalog supports temporary table column listing.

Before

scala> spark.range(10).createOrReplaceTempView("t1")

scala> spark.catalog.listTables().collect()
res1: Array[org.apache.spark.sql.catalog.Table] = Array(Table[name=`t1`, tableType=`TEMPORARY`, isTemporary=`true`])

scala> spark.catalog.listColumns("t1").collect()
org.apache.spark.sql.AnalysisException: Table `t1` does not exist in database `default`.;

After

scala> spark.catalog.listColumns("t1").collect()
res2: Array[org.apache.spark.sql.catalog.Column] = Array(Column[name='id', description='id', dataType='bigint', nullable='false', isPartition='false', isBucket='false'])

How was this patch tested?

Pass the Jenkins tests including a new testcase.

SparkQA · 2016-07-09T11:45:52Z

Test build #62020 has finished for PR 14114 at commit 46196a4.

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

rxin · 2016-07-09T20:59:18Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

@@ -442,6 +442,10 @@ class SessionCatalog(
    name.database.isEmpty && tempTables.contains(formatTableName(name.table))
  }

+  def listTemporaryTableOutput(name: String): Seq[Attribute] = {


rather than doing this, can we make getTableMetadata work for temp tables.

Thank you for review, @rxin .
I'll try again in getTableMetadata.

Ah. I remember that why I do this in this way. Basically, there are two barriers to reach getTableMetadata. Before making change, let me describe here.

Redirecting: listColumns(table) -> listColumns(currentDatabase, tableName)

Table existence failure: requireTableExists(dbName, tableName) in listColumns(currentDatabase, tableName).

Anyway, I'm trying to change the above barriers.

dongjoon-hyun · 2016-07-09T22:56:21Z

Now, getTableMetadata is improved.
Thank you for that advice, @rxin .

SparkQA · 2016-07-09T23:55:54Z

Test build #62035 has finished for PR 14114 at commit 9134a47.

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

SparkQA · 2016-07-10T00:00:57Z

Test build #62036 has finished for PR 14114 at commit be0e69a.

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

SparkQA · 2016-07-10T03:08:10Z

Test build #62039 has finished for PR 14114 at commit bb1204d.

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

dongjoon-hyun · 2016-07-10T03:23:12Z

Hi, @rxin .
Now, it's ready for review again.

dongjoon-hyun · 2016-07-10T21:16:36Z

Hi, @rxin .
Could you review this when you have some time?

rxin · 2016-07-11T00:37:18Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

    val table = formatTableName(name.table)
-    if (name.database.isDefined || !tempTables.contains(table)) {
-      externalCatalog.tableExists(db, table)
+    if (name.database.getOrElse("").length == 0 && tempTables.contains(table)) {


this seems a litlte bit hacky - can we just write it as

if (name.database.isEmpty && tempTables.contains(table)) { // This is a temporary table true } else { ... }

dongjoon-hyun · 2016-07-11T00:53:09Z

Thank you for comments! Both comment are tightly related to each other.
I see what is your point. Yep, implicit use of "" string is not a good idea. I'll fix it again according to the advices.

SparkQA · 2016-07-11T03:45:16Z

Test build #62067 has finished for PR 14114 at commit af6692f.

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

dongjoon-hyun · 2016-07-11T03:51:01Z

Now, it's back for review again.

rxin · 2016-07-11T05:22:39Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

@@ -425,10 +443,11 @@ class SessionCatalog(
  def tableExists(name: TableIdentifier): Boolean = synchronized {


can you update SessionCatalogSuite to reflect this behavior? I think we weren't checking temp tables in the past.

dongjoon-hyun · 2016-07-11T06:08:38Z

Now, SessionCatalogSuite has tableExists and getTableMetadata testcases on temporary views.

SparkQA · 2016-07-11T08:00:35Z

Test build #62077 has finished for PR 14114 at commit cac342e.

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

SparkQA · 2016-07-11T08:04:54Z

Test build #62079 has finished for PR 14114 at commit ac5f5cb.

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

dongjoon-hyun · 2016-07-11T16:08:46Z

Hi, @rxin.
Could you review this again?

hvanhovell · 2016-07-11T17:35:45Z

sql/core/src/main/scala/org/apache/spark/sql/internal/CatalogImpl.scala

@@ -138,7 +138,7 @@ class CatalogImpl(sparkSession: SparkSession) extends Catalog {
   */
  @throws[AnalysisException]("table does not exist")
  override def listColumns(tableName: String): Dataset[Column] = {


This changes the contract of the listColumns(...) function. It now returns either a temporary view or a table in the current database. We have to document this! What happens when we have temporary table with the same name as a table in the current database?

Should't we check if the table exists? (like the other listColumns(...) function)

Thank you again, @hvanhovell

Yep. That is the purpose of this PR, to make the contract consistent with other APIs.

The existence checking here is redundant because it call other listColumns. The callee will check that.

The documentation in org.apache.spark.sql.catalog.Catalog:

/** * Returns a list of columns for the given table in the current database. * * @since 2.0.0 */ @throws[AnalysisException]("table does not exist") def listColumns(tableName: String): Dataset[Column]

Should be updated...

I made mistakes too much. Sorry for that.

I will change like the following: This includes all temporary tables.. It's ambiguous, but it is the current docs.

/** * Returns a list of tables in the current database. * This includes all temporary tables. * * @since 2.0.0 */ def listTables(): Dataset[Table]

…mporary tables

hvanhovell · 2016-07-11T17:58:10Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

+            name = c.name,
+            dataType = c.dataType.catalogString,
+            nullable = c.nullable,
+            comment = Option(c.name)


The metadata can contain the comment, but it is a bit of a PITA to get out:
if (c.metadata.contains("comment")) Some(c.metadata.getString("comment")) else None

So I am fine with leaving this as it is...

What is the reason we need to put the column name in the comment?

dongjoon-hyun · 2016-07-11T18:25:29Z

@hvanhovell .
I rebased and updated one place for isTemporaryTable.
Thank you for in-depth review.

hvanhovell · 2016-07-11T19:28:37Z

LGTM - pending jenkins

SparkQA · 2016-07-11T20:30:14Z

Test build #62111 has finished for PR 14114 at commit e267713.

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

hvanhovell · 2016-07-11T20:44:44Z

Merging to master/2.0. Thanks!

…mporary tables ## What changes were proposed in this pull request? Temporary tables are used frequently, but `spark.catalog.listColumns` does not support those tables. This PR make `SessionCatalog` supports temporary table column listing. **Before** ```scala scala> spark.range(10).createOrReplaceTempView("t1") scala> spark.catalog.listTables().collect() res1: Array[org.apache.spark.sql.catalog.Table] = Array(Table[name=`t1`, tableType=`TEMPORARY`, isTemporary=`true`]) scala> spark.catalog.listColumns("t1").collect() org.apache.spark.sql.AnalysisException: Table `t1` does not exist in database `default`.; ``` **After** ``` scala> spark.catalog.listColumns("t1").collect() res2: Array[org.apache.spark.sql.catalog.Column] = Array(Column[name='id', description='id', dataType='bigint', nullable='false', isPartition='false', isBucket='false']) ``` ## How was this patch tested? Pass the Jenkins tests including a new testcase. Author: Dongjoon Hyun <dongjoon@apache.org> Closes #14114 from dongjoon-hyun/SPARK-16458. (cherry picked from commit 840853e) Signed-off-by: Herman van Hovell <hvanhovell@databricks.com>

SparkQA · 2016-07-11T21:07:37Z

Test build #62113 has finished for PR 14114 at commit 650ead2.

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

dongjoon-hyun · 2016-07-11T21:13:43Z

Thank you for merging, @hvanhovell and @rxin .

SparkQA · 2016-07-11T21:35:10Z

Test build #62114 has finished for PR 14114 at commit d1fa9ec.

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

rxin reviewed Jul 9, 2016
View reviewed changes

dongjoon-hyun mentioned this pull request Jul 10, 2016

[SPARK-16452][SQL] Support basic INFORMATION_SCHEMA #14116

Closed

rxin reviewed Jul 11, 2016
View reviewed changes

hvanhovell reviewed Jul 11, 2016
View reviewed changes

dongjoon-hyun added 5 commits July 11, 2016 10:46

[SPARK-16458][SQL] SessionCatalog should support listColumns for te…

e3c59a0

…mporary tables

Improve getTableMetadata instead.

631ae96

Fix check condition

613bd15

Make private listColumns(TableIdentifier).

ed5301d

Add new testcases.

ab43d47

hvanhovell reviewed Jul 11, 2016
View reviewed changes

Use isTemporaryTable if possible.

e267713

dongjoon-hyun added 2 commits July 11, 2016 12:10

Fix one more.

650ead2

Update docs.

d1fa9ec

asfgit closed this in 840853e Jul 11, 2016

dongjoon-hyun deleted the SPARK-16458 branch July 20, 2016 07:45

gatorsmile mentioned this pull request Sep 6, 2016

[SPARK-17353] [SPARK-16943] [SPARK-16942] [BACKPORT-2.0] [SQL] Fix multiple bugs in CREATE TABLE LIKE command #14946

Closed

		@@ -425,10 +443,11 @@ class SessionCatalog(
		def tableExists(name: TableIdentifier): Boolean = synchronized {

[SPARK-16458][SQL] SessionCatalog should support listColumns for temporary tables #14114

[SPARK-16458][SQL] SessionCatalog should support listColumns for temporary tables #14114

Uh oh!

Conversation

dongjoon-hyun commented Jul 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 9, 2016

Uh oh!

SparkQA commented Jul 9, 2016

Uh oh!

SparkQA commented Jul 10, 2016

Uh oh!

SparkQA commented Jul 10, 2016

Uh oh!

dongjoon-hyun commented Jul 10, 2016

Uh oh!

dongjoon-hyun commented Jul 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

SparkQA commented Jul 11, 2016

Uh oh!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

SparkQA commented Jul 11, 2016

Uh oh!

SparkQA commented Jul 11, 2016

Uh oh!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

hvanhovell commented Jul 11, 2016

Uh oh!

SparkQA commented Jul 11, 2016

Uh oh!

hvanhovell commented Jul 11, 2016

Uh oh!

SparkQA commented Jul 11, 2016

Uh oh!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

SparkQA commented Jul 11, 2016

[SPARK-16458][SQL] SessionCatalog should support `listColumns` for temporary tables #14114

[SPARK-16458][SQL] SessionCatalog should support `listColumns` for temporary tables #14114

dongjoon-hyun commented Jul 9, 2016 •

edited

Loading

dongjoon-hyun Jul 11, 2016 •

edited

Loading