You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2023/05/25 09:07:48 UTC
[GitHub] [spark] beliefer opened a new pull request, #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
beliefer opened a new pull request, #41311:
URL: https://github.com/apache/spark/pull/41311
### What changes were proposed in this pull request?
Currently, the syntax `SHOW CATALOGS LIKE pattern` supports an optional pattern, so as filtered out the expected catalogs.
But the `Catalog.listCatalogs` missing the function both in Catalog API and Connect Catalog API.
In fact, the optional pattern is very useful.
### Why are the changes needed?
This PR want add the optional pattern for `Catalog.listCatalogs`
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41311:
URL: https://github.com/apache/spark/pull/41311#discussion_r1207507283
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala:
##########
@@ -639,4 +639,12 @@ abstract class Catalog {
* @since 3.5.0
*/
def listCatalogs(): Dataset[CatalogMetadata]
+
+ /**
+ * Returns a list of catalogs which name match the specify pattern and available in this
+ * session.
+ *
+ * @since 3.5.0
+ */
+ def listCatalogs(pattern: String): Dataset[CatalogMetadata]
Review Comment:
If we can, I think it's OK too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #41311:
URL: https://github.com/apache/spark/pull/41311#discussion_r1209974242
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala:
##########
@@ -639,4 +639,12 @@ abstract class Catalog {
* @since 3.5.0
*/
def listCatalogs(): Dataset[CatalogMetadata]
+
+ /**
+ * Returns a list of catalogs which name match the specify pattern and available in this
+ * session.
+ *
+ * @since 3.5.0
+ */
+ def listCatalogs(pattern: String): Dataset[CatalogMetadata]
Review Comment:
> If we accept such API, do we prefer to use existing `def listCatalogs()` and change it to `def listCatalogs(pattern: Option[String])`?
it will break the binary compatiablity
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #41311:
URL: https://github.com/apache/spark/pull/41311#issuecomment-1569988329
@cloud-fan @zhengruifeng @amaliujia Thank you for all!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #41311:
URL: https://github.com/apache/spark/pull/41311#issuecomment-1567271355
This looks fine. Do we have similar APIs for listing schemas and tables?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #41311:
URL: https://github.com/apache/spark/pull/41311#discussion_r1210586447
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala:
##########
@@ -639,4 +639,12 @@ abstract class Catalog {
* @since 3.5.0
*/
def listCatalogs(): Dataset[CatalogMetadata]
+
+ /**
+ * Returns a list of catalogs which name match the specify pattern and available in this
+ * session.
+ *
+ * @since 3.5.0
+ */
+ def listCatalogs(pattern: String): Dataset[CatalogMetadata]
Review Comment:
> > If we accept such API, do we prefer to use existing `def listCatalogs()` and change it to `def listCatalogs(pattern: Option[String])`?
>
> it will break the binary compatiablity
ah good point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #41311:
URL: https://github.com/apache/spark/pull/41311#discussion_r1207143459
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala:
##########
@@ -639,4 +639,12 @@ abstract class Catalog {
* @since 3.5.0
*/
def listCatalogs(): Dataset[CatalogMetadata]
+
+ /**
+ * Returns a list of catalogs which name match the specify pattern and available in this
+ * session.
+ *
+ * @since 3.5.0
+ */
+ def listCatalogs(pattern: String): Dataset[CatalogMetadata]
Review Comment:
If we accept such API, do we prefer to use existing `def listCatalogs()` and have `def listCatalogs(pattern: Option[String])`? cc @cloud-fan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #41311:
URL: https://github.com/apache/spark/pull/41311#discussion_r1210745031
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala:
##########
@@ -639,4 +639,12 @@ abstract class Catalog {
* @since 3.5.0
*/
def listCatalogs(): Dataset[CatalogMetadata]
+
+ /**
+ * Returns a list of catalogs which name match the specify pattern and available in this
+ * session.
+ *
+ * @since 3.5.0
+ */
+ def listCatalogs(pattern: String): Dataset[CatalogMetadata]
Review Comment:
@zhengruifeng in fact, what kind of binary compatibility.
The suggested change is still compatibility with existing code? The only way I can think that will break is through reflection (so tried to load `listCatalogs()` but there is no such signature). Is this the binary compatibility referred here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
URL: https://github.com/apache/spark/pull/41311
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #41311:
URL: https://github.com/apache/spark/pull/41311#discussion_r1211053476
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala:
##########
@@ -639,4 +639,12 @@ abstract class Catalog {
* @since 3.5.0
*/
def listCatalogs(): Dataset[CatalogMetadata]
+
+ /**
+ * Returns a list of catalogs which name match the specify pattern and available in this
+ * session.
+ *
+ * @since 3.5.0
+ */
+ def listCatalogs(pattern: String): Dataset[CatalogMetadata]
Review Comment:
I meant that if we change `def listCatalogs()` to `def listCatalogs(pattern: Option[String])`, it breaks the binary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #41311:
URL: https://github.com/apache/spark/pull/41311#issuecomment-1564178433
cc @cloud-fan since this PR aims to add a new catalog API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #41311:
URL: https://github.com/apache/spark/pull/41311#issuecomment-1564124843
ping @HyukjinKwon @zhengruifeng cc @amaliujia
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #41311:
URL: https://github.com/apache/spark/pull/41311#issuecomment-1567639314
> This looks fine. Do we have similar APIs for listing databases and tables?
Not yet. I can create other PR to support them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #41311:
URL: https://github.com/apache/spark/pull/41311#discussion_r1210745031
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala:
##########
@@ -639,4 +639,12 @@ abstract class Catalog {
* @since 3.5.0
*/
def listCatalogs(): Dataset[CatalogMetadata]
+
+ /**
+ * Returns a list of catalogs which name match the specify pattern and available in this
+ * session.
+ *
+ * @since 3.5.0
+ */
+ def listCatalogs(pattern: String): Dataset[CatalogMetadata]
Review Comment:
@zhengruifeng in fact, what kind of binary compatibility that we are discussing?
The suggested change is still compatibility with existing code? The only way I can think that will break is through reflection (so tried to load `listCatalogs()` but there is no such signature). Is this the binary compatibility referred here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #41311:
URL: https://github.com/apache/spark/pull/41311#issuecomment-1566978599
cc @cloud-fan too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #41311:
URL: https://github.com/apache/spark/pull/41311#discussion_r1209973491
##########
python/pyspark/sql/catalog.py:
##########
@@ -119,11 +119,16 @@ def setCurrentCatalog(self, catalogName: str) -> None:
"""
return self._jcatalog.setCurrentCatalog(catalogName)
- def listCatalogs(self) -> List[CatalogMetadata]:
+ def listCatalogs(self, pattern: Optional[str] = None) -> List[CatalogMetadata]:
"""Returns a list of catalogs in this session.
.. versionadded:: 3.4.0
+ Parameters
+ ----------
+ pattern : str
+ The pattern that the catalog name needs to match.
Review Comment:
please add a `versionadded` for this new parameter
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #41311:
URL: https://github.com/apache/spark/pull/41311#discussion_r1207143459
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala:
##########
@@ -639,4 +639,12 @@ abstract class Catalog {
* @since 3.5.0
*/
def listCatalogs(): Dataset[CatalogMetadata]
+
+ /**
+ * Returns a list of catalogs which name match the specify pattern and available in this
+ * session.
+ *
+ * @since 3.5.0
+ */
+ def listCatalogs(pattern: String): Dataset[CatalogMetadata]
Review Comment:
If we accept such API, do we prefer to use existing `def listCatalogs()` and change it to `def listCatalogs(pattern: Option[String])`? cc @cloud-fan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #41311: [SPARK-43792][SQL][PYTHON][CONNECT] Add optional pattern for Catalog.listCatalogs
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #41311:
URL: https://github.com/apache/spark/pull/41311#issuecomment-1569630310
thanks, merging to master!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org