You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/11/29 13:32:05 UTC
[GitHub] [spark] dengziming opened a new pull request, #38838: SPARK-41321: Support target field UnresolvedStar
dengziming opened a new pull request, #38838:
URL: https://github.com/apache/spark/pull/38838
### What changes were proposed in this pull request?
1. Support target field UnresolvedStar
2. UnresolvedStar can be used simultaneously with other expression.
### Why are the changes needed?
This is a necessary feature for UnresolvedStar
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added 2 new unit tests.
--
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 #38838: [SPARK-41321][CONNECT] Support target field for UnresolvedStar
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38838:
URL: https://github.com/apache/spark/pull/38838#discussion_r1035638888
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -360,13 +360,8 @@ class SparkConnectPlanner(session: SparkSession) {
} else {
logical.OneRowRelation()
}
- // TODO: support the target field for *.
val projection =
- if (rel.getExpressionsCount == 1 && rel.getExpressions(0).hasUnresolvedStar) {
- Seq(UnresolvedStar(Option.empty))
- } else {
- rel.getExpressionsList.asScala.map(transformExpression).map(UnresolvedAlias(_))
- }
+ rel.getExpressionsList.asScala.map(transformExpression).map(UnresolvedAlias(_))
Review Comment:
I am actually not sure if Catalyst will smartly see there is a wrapped `Alias` thus ignore the `UnresolvedAlias`. At least it is worth to verify this case.
--
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 closed pull request #38838: [SPARK-41321][CONNECT] Support target field for UnresolvedStar
Posted by GitBox <gi...@apache.org>.
zhengruifeng closed pull request #38838: [SPARK-41321][CONNECT] Support target field for UnresolvedStar
URL: https://github.com/apache/spark/pull/38838
--
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 a diff in pull request #38838: [SPARK-41321][CONNECT] Support target field for UnresolvedStar
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38838:
URL: https://github.com/apache/spark/pull/38838#discussion_r1035643989
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -360,13 +360,8 @@ class SparkConnectPlanner(session: SparkSession) {
} else {
logical.OneRowRelation()
}
- // TODO: support the target field for *.
val projection =
- if (rel.getExpressionsCount == 1 && rel.getExpressions(0).hasUnresolvedStar) {
- Seq(UnresolvedStar(Option.empty))
- } else {
- rel.getExpressionsList.asScala.map(transformExpression).map(UnresolvedAlias(_))
- }
+ rel.getExpressionsList.asScala.map(transformExpression).map(UnresolvedAlias(_))
Review Comment:
I think it will ignore. Can we add a test?
--
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] dengziming commented on a diff in pull request #38838: [SPARK-41321][CONNECT] Support target field for UnresolvedStar
Posted by GitBox <gi...@apache.org>.
dengziming commented on code in PR #38838:
URL: https://github.com/apache/spark/pull/38838#discussion_r1035752572
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -360,13 +360,8 @@ class SparkConnectPlanner(session: SparkSession) {
} else {
logical.OneRowRelation()
}
- // TODO: support the target field for *.
val projection =
- if (rel.getExpressionsCount == 1 && rel.getExpressions(0).hasUnresolvedStar) {
- Seq(UnresolvedStar(Option.empty))
- } else {
- rel.getExpressionsList.asScala.map(transformExpression).map(UnresolvedAlias(_))
- }
+ rel.getExpressionsList.asScala.map(transformExpression).map(UnresolvedAlias(_))
Review Comment:
Thank you for your review @cloud-fan @amaliujia , I have added one test case for it, PTAL.
--
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 pull request #38838: [SPARK-41321][CONNECT] Support target field for UnresolvedStar
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #38838:
URL: https://github.com/apache/spark/pull/38838#issuecomment-1331723568
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 #38838: [SPARK-41321][CONNECT] Support target field for UnresolvedStar
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38838:
URL: https://github.com/apache/spark/pull/38838#discussion_r1035593027
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -360,13 +360,8 @@ class SparkConnectPlanner(session: SparkSession) {
} else {
logical.OneRowRelation()
}
- // TODO: support the target field for *.
val projection =
- if (rel.getExpressionsCount == 1 && rel.getExpressions(0).hasUnresolvedStar) {
- Seq(UnresolvedStar(Option.empty))
- } else {
- rel.getExpressionsList.asScala.map(transformExpression).map(UnresolvedAlias(_))
- }
+ rel.getExpressionsList.asScala.map(transformExpression).map(UnresolvedAlias(_))
Review Comment:
Though the code was written like this, I am seeing a problem that if the Expression is already a `Alias`, we should not wrap it into `UnresolvedAlias` again?
I think you can write a test case like `SELECT 1 as col` then you will find the output schema is not `col` but a auto-generated alias.
##########
connector/connect/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -142,6 +141,9 @@ message Expression {
// UnresolvedStar is used to expand all the fields of a relation or struct.
message UnresolvedStar {
+ // The target of the expansion, either be a table name or struct name, this
Review Comment:
For our current proto style guide. Each field should be marked as either `(Required) or `(Optional)` at the beginning of its comment. See: https://github.com/apache/spark/blob/master/connector/connect/docs/adding-proto-messages.md
--
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 #38838: [SPARK-41321][CONNECT] Support target field for UnresolvedStar
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38838:
URL: https://github.com/apache/spark/pull/38838#discussion_r1035652779
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -360,13 +360,8 @@ class SparkConnectPlanner(session: SparkSession) {
} else {
logical.OneRowRelation()
}
- // TODO: support the target field for *.
val projection =
- if (rel.getExpressionsCount == 1 && rel.getExpressions(0).hasUnresolvedStar) {
- Seq(UnresolvedStar(Option.empty))
- } else {
- rel.getExpressionsList.asScala.map(transformExpression).map(UnresolvedAlias(_))
- }
+ rel.getExpressionsList.asScala.map(transformExpression).map(UnresolvedAlias(_))
Review Comment:
+1 if there is no such test then add one.
--
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] AmplabJenkins commented on pull request #38838: [SPARK-41321][CONNECT] Support target field for UnresolvedStar
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38838:
URL: https://github.com/apache/spark/pull/38838#issuecomment-1331426085
Can one of the admins verify this patch?
--
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 #38838: [SPARK-41321][CONNECT] Support target field for UnresolvedStar
Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on PR #38838:
URL: https://github.com/apache/spark/pull/38838#issuecomment-1333078020
@dengziming thank you for working on this!
merged into 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