You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "michaelzhan-db (via GitHub)" <gi...@apache.org> on 2023/10/06 01:05:31 UTC
[PR] [SPARK-45418] Change current_database() column alias to current_schema() [spark]
michaelzhan-db opened a new pull request, #43235:
URL: https://github.com/apache/spark/pull/43235
### What changes were proposed in this pull request?
Change column alias for current_database() to current_schema.
### Why are the changes needed?
To better align with preferred usage of schema rather than database for three part namespace.
### Does this PR introduce _any_ user-facing change?
Yes, `current_database()` column alias is now `current_schema()`.
### How was this patch tested?
Unit tests pass.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [SPARK-45418][SQL][PYTHON][CONNECT] Change current_database() column alias to current_schema() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #43235:
URL: https://github.com/apache/spark/pull/43235#discussion_r1349943772
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -189,7 +189,7 @@ object AssertTrue {
case class CurrentDatabase() extends LeafExpression with Unevaluable {
override def dataType: DataType = StringType
override def nullable: Boolean = false
- override def prettyName: String = "current_database"
+ override def prettyName: String = "current_schema"
Review Comment:
AFAIK schema is a more standard name to reference to a namespace that keeps database objects.
--
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
Re: [PR] [SPARK-45418] Change current_database() column alias to current_schema() [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #43235:
URL: https://github.com/apache/spark/pull/43235#issuecomment-1752232965
also cc @cloud-fan @peter-toth
--
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
Re: [PR] [SPARK-45418][SQL][PYTHON][CONNECT] Change current_database() column alias to current_schema() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #43235:
URL: https://github.com/apache/spark/pull/43235#discussion_r1349942111
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -189,7 +189,7 @@ object AssertTrue {
case class CurrentDatabase() extends LeafExpression with Unevaluable {
override def dataType: DataType = StringType
override def nullable: Boolean = false
- override def prettyName: String = "current_database"
+ override def prettyName: String = "current_schema"
Review Comment:
`CurrentDatabase` was there since 1.6.0 and seems safer to not rename it now. The expression class name is not user-facing anyway.
--
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
Re: [PR] [SPARK-45418][SQL][PYTHON][CONNECT] Change current_database() column alias to current_schema() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #43235:
URL: https://github.com/apache/spark/pull/43235#issuecomment-1760569751
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
Re: [PR] [SPARK-45418] Change current_database() column alias to current_schema() [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43235:
URL: https://github.com/apache/spark/pull/43235#discussion_r1349449434
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -189,7 +189,7 @@ object AssertTrue {
case class CurrentDatabase() extends LeafExpression with Unevaluable {
override def dataType: DataType = StringType
override def nullable: Boolean = false
- override def prettyName: String = "current_database"
+ override def prettyName: String = "current_schema"
Review Comment:
If we change current_database to current_schema, we should rename `CurrentDatabase` as `CurrentSchema`.
How about using `current_database` as a unified name?
--
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
Re: [PR] [SPARK-45418][SQL][PYTHON][CONNECT] Change current_database() column alias to current_schema() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #43235: [SPARK-45418][SQL][PYTHON][CONNECT] Change current_database() column alias to current_schema()
URL: https://github.com/apache/spark/pull/43235
--
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
Re: [PR] [SPARK-45418] Change current_database() column alias to current_schema() [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43235:
URL: https://github.com/apache/spark/pull/43235#discussion_r1349449434
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -189,7 +189,7 @@ object AssertTrue {
case class CurrentDatabase() extends LeafExpression with Unevaluable {
override def dataType: DataType = StringType
override def nullable: Boolean = false
- override def prettyName: String = "current_database"
+ override def prettyName: String = "current_schema"
Review Comment:
If we change `current_database` to `current_schema`, we should rename `CurrentDatabase` to `CurrentSchema`.
How about using `current_database` as a unified name?
--
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
Re: [PR] [SPARK-45418][SQL][PYTHON][CONNECT] Change current_database() column alias to current_schema() [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43235:
URL: https://github.com/apache/spark/pull/43235#discussion_r1350068798
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -189,7 +189,7 @@ object AssertTrue {
case class CurrentDatabase() extends LeafExpression with Unevaluable {
override def dataType: DataType = StringType
override def nullable: Boolean = false
- override def prettyName: String = "current_database"
+ override def prettyName: String = "current_schema"
Review Comment:
I know this PR want to use the name `schema` here, it's okay. However, it caused inconsistent between the class name of the `CurrentDatabase` and its `prettyName`. Is this a problem? If not, then it's okay.
--
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
Re: [PR] [SPARK-45418] Change current_database() column alias to current_schema() [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43235:
URL: https://github.com/apache/spark/pull/43235#discussion_r1349459696
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -189,7 +189,7 @@ object AssertTrue {
case class CurrentDatabase() extends LeafExpression with Unevaluable {
override def dataType: DataType = StringType
override def nullable: Boolean = false
- override def prettyName: String = "current_database"
+ override def prettyName: String = "current_schema"
Review Comment:
cc @zhengruifeng @HyukjinKwon
--
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
Re: [PR] [SPARK-45418][SQL][PYTHON][CONNECT] Change current_database() column alias to current_schema() [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43235:
URL: https://github.com/apache/spark/pull/43235#discussion_r1349977125
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -189,7 +189,7 @@ object AssertTrue {
case class CurrentDatabase() extends LeafExpression with Unevaluable {
override def dataType: DataType = StringType
override def nullable: Boolean = false
- override def prettyName: String = "current_database"
+ override def prettyName: String = "current_schema"
Review Comment:
So, how about using `current_database` as a unified name? it is consistent with class name.
--
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
Re: [PR] [SPARK-45418] Change current_database() column alias to current_schema() [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43235:
URL: https://github.com/apache/spark/pull/43235#discussion_r1349449434
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -189,7 +189,7 @@ object AssertTrue {
case class CurrentDatabase() extends LeafExpression with Unevaluable {
override def dataType: DataType = StringType
override def nullable: Boolean = false
- override def prettyName: String = "current_database"
+ override def prettyName: String = "current_schema"
Review Comment:
If we change `current_database` to `current_schema`, we should rename `CurrentDatabase` as `CurrentSchema`.
How about using `current_database` as a unified name?
--
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
Re: [PR] [SPARK-45418][SQL][PYTHON][CONNECT] Change current_database() column alias to current_schema() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #43235:
URL: https://github.com/apache/spark/pull/43235#discussion_r1350041686
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -189,7 +189,7 @@ object AssertTrue {
case class CurrentDatabase() extends LeafExpression with Unevaluable {
override def dataType: DataType = StringType
override def nullable: Boolean = false
- override def prettyName: String = "current_database"
+ override def prettyName: String = "current_schema"
Review Comment:
Sorry I'm a bit confused. This PR tries to use the name `schema` more widely as it's more SQL standard. Are you saying no to this PR and want to stick with `database`?
--
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
Re: [PR] [SPARK-45418][SQL][PYTHON][CONNECT] Change current_database() column alias to current_schema() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #43235:
URL: https://github.com/apache/spark/pull/43235#discussion_r1350276899
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -189,7 +189,7 @@ object AssertTrue {
case class CurrentDatabase() extends LeafExpression with Unevaluable {
override def dataType: DataType = StringType
override def nullable: Boolean = false
- override def prettyName: String = "current_database"
+ override def prettyName: String = "current_schema"
Review Comment:
It's fine, `TruncTimestamp` overrides `prettyName` as `date_trunc`
--
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