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.
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
[SPARK-43961][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listTables #41461
Conversation
I think this makes sense to add the functionality to the Scala/Python APIs since it's already there in the SQL API. I also appreciate the cleanup of the duplicated code as well in this PR :) |
@@ -77,6 +77,8 @@ message ListDatabases { | |||
message ListTables { | |||
// (Optional) | |||
optional string db_name = 1; | |||
// (Optional) The pattern that the table name needs to match | |||
optional string pattern = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a test case for Spark Connect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added test cases in CatalogSuite
.
* @since 3.5.0 | ||
*/ | ||
@throws[AnalysisException]("database does not exist") | ||
override def listTables(dbName: String, pattern: String): Dataset[Table] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for the impl we can have one implementation of listTables.
def listTablesInternal(dbName: String, pattern: Optiona[String])
override def listTables(dbName: String) = listTablesInternal(dbName, None)
override def listTables(dbName: String, pattern: String) = listTablesInternal(dbName, Some(pattern))
@@ -403,6 +397,19 @@ class CatalogImpl(sparkSession: SparkSession) extends Catalog { | |||
} | |||
} | |||
|
|||
private def wrapNamespace(dbName: String): Seq[String] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this method should be named resolveNamespace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thanks, merging to master! |
@cloud-fan @amaliujia @HyukjinKwon @holdenk Thank you! |
…listTables ### What changes were proposed in this pull request? Currently, the syntax `SHOW TABLES LIKE pattern` supports an optional pattern, so as filtered out the expected tables. But the `Catalog.listTables` missing the function both in Catalog API and Connect Catalog API. In fact, the optional pattern is very useful. This PR also extracts the common `wrapNamespace` to clean up the duplicated code. ### Why are the changes needed? This PR want add the optional pattern for `Catalog.listTables`. ### Does this PR introduce _any_ user-facing change? 'No'. New feature. ### How was this patch tested? New test cases. Closes apache#41461 from beliefer/SPARK-43961. Authored-by: Jiaan Geng <beliefer@163.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
Currently, the syntax
SHOW TABLES LIKE pattern
supports an optional pattern, so as filtered out the expected tables.But the
Catalog.listTables
missing the function both in Catalog API and Connect Catalog API.In fact, the optional pattern is very useful.
This PR also extracts the common
wrapNamespace
to clean up the duplicated code.Why are the changes needed?
This PR want add the optional pattern for
Catalog.listTables
.Does this PR introduce any user-facing change?
'No'.
New feature.
How was this patch tested?
New test cases.