You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhengruifeng (via GitHub)" <gi...@apache.org> on 2023/02/14 07:28:56 UTC
[GitHub] [spark] zhengruifeng opened a new pull request, #40008: [SPARK-42431][CONNECT] Avoid calling `output` before analysis in `Union`
zhengruifeng opened a new pull request, #40008:
URL: https://github.com/apache/spark/pull/40008
### What changes were proposed in this pull request?
Avoid calling `output` before analysis;
Remove the usage of optimizer `CombineUnions`, since it may discard the `PLAN_ID_TAG`
### Why are the changes needed?
it is not expected to calling `output` and apply optimizer rules before analysis
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
updated UT
--
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 #40008: [SPARK-42431][CONNECT] Avoid calling `LogicalPlan.output` before analysis in `Union`
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on PR #40008:
URL: https://github.com/apache/spark/pull/40008#issuecomment-1430025642
late LGTM
--
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] HyukjinKwon closed pull request #40008: [SPARK-42431][CONNECT] Avoid calling `LogicalPlan.output` before analysis in `Union`
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #40008: [SPARK-42431][CONNECT] Avoid calling `LogicalPlan.output` before analysis in `Union`
URL: https://github.com/apache/spark/pull/40008
--
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] HyukjinKwon commented on pull request #40008: [SPARK-42431][CONNECT] Avoid calling `LogicalPlan.output` before analysis in `Union`
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #40008:
URL: https://github.com/apache/spark/pull/40008#issuecomment-1429707666
Merged to master and branch-3.4.
--
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] hvanhovell commented on a diff in pull request #40008: [SPARK-42431][CONNECT] Avoid calling `LogicalPlan.output` before analysis in `Union`
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #40008:
URL: https://github.com/apache/spark/pull/40008#discussion_r1106529796
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -1237,37 +1236,37 @@ class SparkConnectPlanner(val session: SparkSession) {
}
private def transformSetOperation(u: proto.SetOperation): LogicalPlan = {
- assert(u.hasLeftInput && u.hasRightInput, "Union must have 2 inputs")
+ if (!u.hasLeftInput || !u.hasRightInput) {
+ throw InvalidPlanInput("Set operation must have 2 inputs")
+ }
+ val leftPlan = transformRelation(u.getLeftInput)
+ val rightPlan = transformRelation(u.getRightInput)
+ val isAll = if (u.hasIsAll) u.getIsAll else false
u.getSetOpType match {
case proto.SetOperation.SetOpType.SET_OP_TYPE_EXCEPT =>
if (u.getByName) {
throw InvalidPlanInput("Except does not support union_by_name")
}
- Except(transformRelation(u.getLeftInput), transformRelation(u.getRightInput), u.getIsAll)
+ Except(leftPlan, rightPlan, isAll)
case proto.SetOperation.SetOpType.SET_OP_TYPE_INTERSECT =>
if (u.getByName) {
throw InvalidPlanInput("Intersect does not support union_by_name")
}
- Intersect(
- transformRelation(u.getLeftInput),
- transformRelation(u.getRightInput),
- u.getIsAll)
+ Intersect(leftPlan, rightPlan, isAll)
case proto.SetOperation.SetOpType.SET_OP_TYPE_UNION =>
if (!u.getByName && u.getAllowMissingColumns) {
throw InvalidPlanInput(
"UnionByName `allowMissingCol` can be true only if `byName` is true.")
}
- val combinedUnion = CombineUnions(
- Union(
- Seq(transformRelation(u.getLeftInput), transformRelation(u.getRightInput)),
- byName = u.getByName,
- allowMissingCol = u.getAllowMissingColumns))
- if (u.getIsAll) {
- combinedUnion
+ val union = Union(Seq(leftPlan, rightPlan), u.getByName, u.getAllowMissingColumns)
+ if (isAll) {
+ union
} else {
- logical.Deduplicate(combinedUnion.output, combinedUnion)
+ val analyzed = new QueryExecution(session, union).analyzed
+ Deduplicate(analyzed.output, analyzed)
Review Comment:
Is there away we can defer analysis here? We could just use `Distinct` 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] zhengruifeng commented on a diff in pull request #40008: [SPARK-42431][CONNECT] Avoid calling `LogicalPlan.output` before analysis in `Union`
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #40008:
URL: https://github.com/apache/spark/pull/40008#discussion_r1106628436
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -1237,37 +1236,37 @@ class SparkConnectPlanner(val session: SparkSession) {
}
private def transformSetOperation(u: proto.SetOperation): LogicalPlan = {
- assert(u.hasLeftInput && u.hasRightInput, "Union must have 2 inputs")
+ if (!u.hasLeftInput || !u.hasRightInput) {
+ throw InvalidPlanInput("Set operation must have 2 inputs")
+ }
+ val leftPlan = transformRelation(u.getLeftInput)
+ val rightPlan = transformRelation(u.getRightInput)
+ val isAll = if (u.hasIsAll) u.getIsAll else false
u.getSetOpType match {
case proto.SetOperation.SetOpType.SET_OP_TYPE_EXCEPT =>
if (u.getByName) {
throw InvalidPlanInput("Except does not support union_by_name")
}
- Except(transformRelation(u.getLeftInput), transformRelation(u.getRightInput), u.getIsAll)
+ Except(leftPlan, rightPlan, isAll)
case proto.SetOperation.SetOpType.SET_OP_TYPE_INTERSECT =>
if (u.getByName) {
throw InvalidPlanInput("Intersect does not support union_by_name")
}
- Intersect(
- transformRelation(u.getLeftInput),
- transformRelation(u.getRightInput),
- u.getIsAll)
+ Intersect(leftPlan, rightPlan, isAll)
case proto.SetOperation.SetOpType.SET_OP_TYPE_UNION =>
if (!u.getByName && u.getAllowMissingColumns) {
throw InvalidPlanInput(
"UnionByName `allowMissingCol` can be true only if `byName` is true.")
}
- val combinedUnion = CombineUnions(
- Union(
- Seq(transformRelation(u.getLeftInput), transformRelation(u.getRightInput)),
- byName = u.getByName,
- allowMissingCol = u.getAllowMissingColumns))
- if (u.getIsAll) {
- combinedUnion
+ val union = Union(Seq(leftPlan, rightPlan), u.getByName, u.getAllowMissingColumns)
+ if (isAll) {
+ union
} else {
- logical.Deduplicate(combinedUnion.output, combinedUnion)
+ val analyzed = new QueryExecution(session, union).analyzed
+ Deduplicate(analyzed.output, analyzed)
Review Comment:
https://github.com/apache/spark/pull/40029
--
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 #40008: [SPARK-42431][CONNECT] Avoid calling `LogicalPlan.output` before analysis in `Union`
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #40008:
URL: https://github.com/apache/spark/pull/40008#discussion_r1106557435
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -1237,37 +1236,37 @@ class SparkConnectPlanner(val session: SparkSession) {
}
private def transformSetOperation(u: proto.SetOperation): LogicalPlan = {
- assert(u.hasLeftInput && u.hasRightInput, "Union must have 2 inputs")
+ if (!u.hasLeftInput || !u.hasRightInput) {
+ throw InvalidPlanInput("Set operation must have 2 inputs")
+ }
+ val leftPlan = transformRelation(u.getLeftInput)
+ val rightPlan = transformRelation(u.getRightInput)
+ val isAll = if (u.hasIsAll) u.getIsAll else false
u.getSetOpType match {
case proto.SetOperation.SetOpType.SET_OP_TYPE_EXCEPT =>
if (u.getByName) {
throw InvalidPlanInput("Except does not support union_by_name")
}
- Except(transformRelation(u.getLeftInput), transformRelation(u.getRightInput), u.getIsAll)
+ Except(leftPlan, rightPlan, isAll)
case proto.SetOperation.SetOpType.SET_OP_TYPE_INTERSECT =>
if (u.getByName) {
throw InvalidPlanInput("Intersect does not support union_by_name")
}
- Intersect(
- transformRelation(u.getLeftInput),
- transformRelation(u.getRightInput),
- u.getIsAll)
+ Intersect(leftPlan, rightPlan, isAll)
case proto.SetOperation.SetOpType.SET_OP_TYPE_UNION =>
if (!u.getByName && u.getAllowMissingColumns) {
throw InvalidPlanInput(
"UnionByName `allowMissingCol` can be true only if `byName` is true.")
}
- val combinedUnion = CombineUnions(
- Union(
- Seq(transformRelation(u.getLeftInput), transformRelation(u.getRightInput)),
- byName = u.getByName,
- allowMissingCol = u.getAllowMissingColumns))
- if (u.getIsAll) {
- combinedUnion
+ val union = Union(Seq(leftPlan, rightPlan), u.getByName, u.getAllowMissingColumns)
+ if (isAll) {
+ union
} else {
- logical.Deduplicate(combinedUnion.output, combinedUnion)
+ val analyzed = new QueryExecution(session, union).analyzed
+ Deduplicate(analyzed.output, analyzed)
Review Comment:
Thanks, will send a followup for it.
--
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