Skip to content

[SPARK-20198] [SQL] Remove the inconsistency in table/function name conventions in SparkSession.Catalog APIs#17518

Closed
gatorsmile wants to merge 4 commits intoapache:masterfrom
gatorsmile:tableIdentifier
Closed

[SPARK-20198] [SQL] Remove the inconsistency in table/function name conventions in SparkSession.Catalog APIs#17518
gatorsmile wants to merge 4 commits intoapache:masterfrom
gatorsmile:tableIdentifier

Conversation

@gatorsmile
Copy link
Copy Markdown
Member

@gatorsmile gatorsmile commented Apr 3, 2017

What changes were proposed in this pull request?

Observed by @felixcheung , in SparkSession.Catalog APIs, we have different conventions/rules for table/function identifiers/names. Most APIs accept the qualified name (i.e., databaseName.tableName or databaseName.functionName). However, the following five APIs do not accept it.

  • def listColumns(tableName: String): Dataset[Column]
  • def getTable(tableName: String): Table
  • def getFunction(functionName: String): Function
  • def tableExists(tableName: String): Boolean
  • def functionExists(functionName: String): Boolean

To make them consistent with the other Catalog APIs, this PR does the changes, updates the function/API comments and adds the @params to clarify the inputs we allow.

How was this patch tested?

Added the test cases .

@gatorsmile gatorsmile changed the title [SPARK-20198] [SQL] Remove the inconsistency when table/function name definitions in SparkSession.Catalog APIs [SPARK-20198] [SQL] Remove the inconsistency in table/function name conventions in SparkSession.Catalog APIs Apr 3, 2017
@SparkQA
Copy link
Copy Markdown

SparkQA commented Apr 3, 2017

Test build #75473 has finished for PR 17518 at commit dc9fefa.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

gatorsmile
gatorsmile commented Apr 3, 2017
View reviewed changes
Comment thread sql/core/src/test/scala/org/apache/spark/sql/internal/CatalogSuite.scala
assert(spark.catalog.tableExists("tbl_y"))

// Unable to find the table, although the temp view with the given name exists
assert(!spark.catalog.tableExists(db, "tbl_x"))
Copy link
Copy Markdown
Member Author

@gatorsmile gatorsmile Apr 3, 2017

Choose a reason for hiding this comment

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

This new test case is not related to this PR. Just to improve the existing test case coverage.

gatorsmile
gatorsmile commented Apr 3, 2017
View reviewed changes
Comment thread sql/core/src/test/scala/org/apache/spark/sql/internal/CatalogSuite.scala
assert(spark.catalog.functionExists("fn2"))

// Unable to find the function, although the temp function with the given name exists
assert(!spark.catalog.functionExists(db, "fn1"))
Copy link
Copy Markdown
Member Author

@gatorsmile gatorsmile Apr 3, 2017

Choose a reason for hiding this comment

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

This new test case is not related to this PR. Just to improve the existing test case coverage.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Apr 3, 2017

Test build #75474 has finished for PR 17518 at commit b3e81b4.

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

@gatorsmile
Copy link
Copy Markdown
Member Author

gatorsmile commented Apr 3, 2017

cc @rxin @hvanhovell @cloud-fan @felixcheung

@rxin
Copy link
Copy Markdown
Contributor

rxin commented Apr 3, 2017

Is this an API change or just a documentation change? The title suggests you are changing public facing APIs?

@gatorsmile
Copy link
Copy Markdown
Member Author

gatorsmile commented Apr 3, 2017 •
edited
Loading

@rxin This has an API change. After this PR, the following APIs will accept the qualified table/function names.

  • def listColumns(tableName: String): Dataset[Column]
  • def getTable(tableName: String): Table
  • def getFunction(functionName: String): Function
  • def tableExists(tableName: String): Boolean
  • def functionExists(functionName: String): Boolean

For example, before this PR, listColumns("db1.tab") will search a table having the name db1.tab. After this PR, it will search the table having the name tab in the database db1

@felixcheung felixcheung mentioned this pull request Apr 4, 2017
Closed
cloud-fan
cloud-fan reviewed Apr 4, 2017
View reviewed changes
Comment thread sql/core/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala
* @return true if the view is dropped successfully, false otherwise.
* @since 2.1.0
*/
def dropGlobalTempView(viewName: String): Boolean
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Apr 4, 2017

Choose a reason for hiding this comment

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

hmm, just realized we don't have dropTable in Catalog...

@cloud-fan
Copy link
Copy Markdown
Contributor

cloud-fan commented Apr 4, 2017

@rxin I think these API changes make sense, it's more intuitive to return columns for tbl1 in db1 when calling listColumns("db1.tbl1"), instead of a special table named db1.tbl1, the previous behavior is kind of bugy and not consistent with other interfaces in Catalog.

LGTM. merging to master!

@asfgit asfgit closed this in 26e7bca Apr 4, 2017
@smurching smurching mentioned this pull request Dec 4, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fan cloud-fan cloud-fan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gatorsmile @SparkQA @rxin @cloud-fan