[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r77544824
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
val db =
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
val table = formatTableName(name.table)
-requireDbExists(db)
-requireTableExists(TableIdentifier(table, Some(db)))
-externalCatalog.getTable(db, table)
+val tid = TableIdentifier(table)
+if (name.database.isEmpty && isTemporaryTable(tid)) {
+ CatalogTable(
+identifier = tid,
+tableType = CatalogTableType.VIEW,
+storage = CatalogStorageFormat.empty,
+schema = tempTables(table).output.map { c =>
+ CatalogColumn(
+name = c.name,
+dataType = c.dataType.catalogString,
+nullable = c.nullable,
+comment = Option(c.name)
--- End diff --
What is the reason we need to put the column name in the comment?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/14114 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70319730
--- Diff:
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] = {
--- End diff --
I made mistakes too much. Sorry for that.
I will change like the following.
```scala
/**
* Returns a list of tables in the current database.
* This includes all temporary tables.
*
* @since 2.0.0
*/
def listTables(): Dataset[Table]
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70319495
--- Diff:
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] = {
--- End diff --
What is the proper wording, `Spark temporary views`?
```
scala> spark.range(10).createOrReplaceTempView("t1")
scala> spark.catalog.listTables().show
+++---+-+---+
|name|database|description|tableType|isTemporary|
+++---+-+---+
| t1|null| null|TEMPORARY| true|
+++---+-+---+
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70319254
--- Diff:
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] = {
--- End diff --
Oh, right. We should update here too.
```
/**
* Returns a list of tables in the specified database.
* This includes all temporary tables.
*
* @since 2.0.0
*/
@throws[AnalysisException]("database does not exist")
def listTables(dbName: String): Dataset[Table]
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70318287
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
val db =
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
val table = formatTableName(name.table)
-requireDbExists(db)
-requireTableExists(TableIdentifier(table, Some(db)))
-externalCatalog.getTable(db, table)
+val tid = TableIdentifier(table)
+if (name.database.isEmpty && isTemporaryTable(tid)) {
--- End diff --
Oops. My bad. Indeed. I thought only one direction.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70317807
--- Diff:
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] = {
--- End diff --
The documentation in org.apache.spark.sql.catalog.Catalog:
```scala
/**
* 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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70314737
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
val db =
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
val table = formatTableName(name.table)
-requireDbExists(db)
-requireTableExists(TableIdentifier(table, Some(db)))
-externalCatalog.getTable(db, table)
+val tid = TableIdentifier(table)
+if (name.database.isEmpty && isTemporaryTable(tid)) {
--- End diff --
```scala
val table = formatTableName(name.table)
val tid = TableIdentifier(table)
name.database.isEmpty && isTemporaryTable(tid)
```
...is the same as...
```scala
isTemporaryTable(name)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70309422
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
val db =
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
val table = formatTableName(name.table)
-requireDbExists(db)
-requireTableExists(TableIdentifier(table, Some(db)))
-externalCatalog.getTable(db, table)
+val tid = TableIdentifier(table)
+if (name.database.isEmpty && isTemporaryTable(tid)) {
--- End diff --
Hi, `isTemporaryTable` checks `tid.database.isEmpty`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70309114
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
val db =
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
val table = formatTableName(name.table)
-requireDbExists(db)
-requireTableExists(TableIdentifier(table, Some(db)))
-externalCatalog.getTable(db, table)
+val tid = TableIdentifier(table)
+if (name.database.isEmpty && isTemporaryTable(tid)) {
--- End diff --
Oops. @hvanhovell . I found the original reason.
The logic of `isTemporaryTable` only lookup `current database`. We should
support the following.
```
val m3 = intercept[AnalysisException] {
catalog.getTableMetadata(TableIdentifier("view1", Some("default")))
}.getMessage
assert(m3.contains("Table or view 'view1' not found in database
'default'"))
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70307717
--- Diff:
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 {
val db = formatDatabaseName(name.database.getOrElse(currentDb))
val table = formatTableName(name.table)
-if (name.database.isDefined || !tempTables.contains(table)) {
- externalCatalog.tableExists(db, table)
+if (name.database.isEmpty && tempTables.contains(table)) {
--- End diff --
Yep. What I mean I'll follow your advice. I misunderstood the function
definition. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70307372
--- Diff:
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 {
val db = formatDatabaseName(name.database.getOrElse(currentDb))
val table = formatTableName(name.table)
-if (name.database.isDefined || !tempTables.contains(table)) {
- externalCatalog.tableExists(db, table)
+if (name.database.isEmpty && tempTables.contains(table)) {
--- End diff --
Huh? The definition is exactly the same:
```scala
def isTemporaryTable(name: TableIdentifier): Boolean = synchronized {
name.database.isEmpty && tempTables.contains(formatTableName(name.table))
}
```
... and ...
```scala
val table = formatTableName(name.table)
if (name.database.isEmpty && tempTables.contains(table)) {
...
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70306857
--- Diff:
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 {
val db = formatDatabaseName(name.database.getOrElse(currentDb))
val table = formatTableName(name.table)
-if (name.database.isDefined || !tempTables.contains(table)) {
- externalCatalog.tableExists(db, table)
+if (name.database.isEmpty && tempTables.contains(table)) {
--- End diff --
I'll fix this, too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70306434
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
val db =
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
val table = formatTableName(name.table)
-requireDbExists(db)
-requireTableExists(TableIdentifier(table, Some(db)))
-externalCatalog.getTable(db, table)
+val tid = TableIdentifier(table)
+if (name.database.isEmpty && isTemporaryTable(tid)) {
--- End diff --
The only reason not to use that is just its `synchronized`.
But, I'll update according to your advice.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70306040
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
val db =
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
val table = formatTableName(name.table)
-requireDbExists(db)
-requireTableExists(TableIdentifier(table, Some(db)))
-externalCatalog.getTable(db, table)
+val tid = TableIdentifier(table)
+if (name.database.isEmpty && isTemporaryTable(tid)) {
--- End diff --
Oh. I see what you mean now.
Hmm.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70305902
--- Diff:
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 {
val db = formatDatabaseName(name.database.getOrElse(currentDb))
val table = formatTableName(name.table)
-if (name.database.isDefined || !tempTables.contains(table)) {
- externalCatalog.tableExists(db, table)
+if (name.database.isEmpty && tempTables.contains(table)) {
--- End diff --
The second one have `database` name.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70305811
--- Diff:
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 {
val db = formatDatabaseName(name.database.getOrElse(currentDb))
val table = formatTableName(name.table)
-if (name.database.isDefined || !tempTables.contains(table)) {
- externalCatalog.tableExists(db, table)
+if (name.database.isEmpty && tempTables.contains(table)) {
--- End diff --
Yep. I thought like that at the first commit. But, there is a testcase
violation. The following case should be allowed.
```
CREATE TEMPORARY VIEW t ...
CREATE VIEW t
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70305796
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
val db =
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
val table = formatTableName(name.table)
-requireDbExists(db)
-requireTableExists(TableIdentifier(table, Some(db)))
-externalCatalog.getTable(db, table)
+val tid = TableIdentifier(table)
+if (name.database.isEmpty && isTemporaryTable(tid)) {
+ CatalogTable(
+identifier = tid,
+tableType = CatalogTableType.VIEW,
+storage = CatalogStorageFormat.empty,
+schema = tempTables(table).output.map { c =>
+ CatalogColumn(
+name = c.name,
+dataType = c.dataType.catalogString,
+nullable = c.nullable,
+comment = Option(c.name)
--- End diff --
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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70305351
--- Diff:
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] = {
--- End diff --
Thank you again, @hvanhovell
1. Yep. That is the purpose of this PR, to make the contract consistent
with other APIs.
2. The existence checking here is redundant because it call other
`listColumns`. The callee will check that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70303041
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
val db =
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
val table = formatTableName(name.table)
-requireDbExists(db)
-requireTableExists(TableIdentifier(table, Some(db)))
-externalCatalog.getTable(db, table)
+val tid = TableIdentifier(table)
+if (name.database.isEmpty && isTemporaryTable(tid)) {
--- End diff --
`isTemporaryTable` also checks `name.database.isEmpty`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70302921
--- Diff:
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 {
val db = formatDatabaseName(name.database.getOrElse(currentDb))
val table = formatTableName(name.table)
-if (name.database.isDefined || !tempTables.contains(table)) {
- externalCatalog.tableExists(db, table)
+if (name.database.isEmpty && tempTables.contains(table)) {
--- End diff --
Shouldn't we use isTemporaryTable(...) here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70302158
--- Diff:
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] = {
--- End diff --
Should't we check if the table exists? (like the other listColumns(...)
function)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70301759
--- Diff:
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] = {
--- End diff --
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70204413
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
--- End diff --
Yep.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70204393
--- Diff:
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 {
--- End diff --
Sure.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70203557
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -246,9 +246,27 @@ class SessionCatalog(
def getTableMetadata(name: TableIdentifier): CatalogTable = {
--- End diff --
same thing here - update SessionCatalogSuite.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70203553
--- Diff:
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 {
--- End diff --
can you update SessionCatalogSuite to reflect this behavior? I think we
weren't checking temp tables in the past.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70191915
--- Diff:
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] = {
-listColumns(currentDatabase, tableName)
+listColumns("", tableName)
--- End diff --
i see why you did it this way up there. i think you can just create a
private method here listColumns that takes a TableIdentifier, and then we are
good to go?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70191775
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
---
@@ -423,12 +442,13 @@ class SessionCatalog(
* contain the table.
*/
def tableExists(name: TableIdentifier): Boolean = synchronized {
-val db = formatDatabaseName(name.database.getOrElse(currentDb))
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)) {
--- End diff --
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 {
...
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70172672
--- Diff:
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] = {
--- End diff --
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.
1. Redirecting: `listColumns(table)` -> `listColumns(currentDatabase,
tableName)`
2. Table existence failure: `requireTableExists(dbName, tableName)` in
`listColumns(currentDatabase, tableName)`.
Anyway, I'm trying to change the above barriers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70171997
--- Diff:
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] = {
--- End diff --
Thank you for review, @rxin .
I'll try again in `getTableMetadata`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14114#discussion_r70171976
--- Diff:
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] = {
--- End diff --
rather than doing this, can we make getTableMetadata work for temp tables.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #14114: [SPARK-16458][SQL] SessionCatalog should support ...
GitHub user dongjoon-hyun opened a pull request:
https://github.com/apache/spark/pull/14114
[SPARK-16458][SQL] SessionCatalog should support `listColumns` for
temporary 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dongjoon-hyun/spark SPARK-16458
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/14114.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #14114
commit 46196a44d18426a107b480b8157c5accedecbbd1
Author: Dongjoon Hyun
Date: 2016-07-09T09:54:39Z
[SPARK-16458][SQL] SessionCatalog should support `listColumns` for
temporary tables
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
