-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-32241][SQL] Remove empty children of union #29053
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
Changes from 3 commits
71b28fd
f9c0a3d
b454cd7
e10fe02
cea0a48
05a90ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,8 +50,24 @@ object PropagateEmptyRelation extends Rule[LogicalPlan] with PredicateHelper wit | |
| override def conf: SQLConf = SQLConf.get | ||
|
|
||
| def apply(plan: LogicalPlan): LogicalPlan = plan transformUp { | ||
| case p: Union if p.children.forall(isEmptyLocalRelation) => | ||
| empty(p) | ||
| case p @ Union(children) if children.exists(isEmptyLocalRelation) => | ||
| val newChildren = children.filterNot(isEmptyLocalRelation) | ||
| if (newChildren.isEmpty) { | ||
| empty(p) | ||
| } else { | ||
|
dongjoon-hyun marked this conversation as resolved.
|
||
| val newPlan = if (newChildren.size > 1) Union(newChildren) else newChildren.head | ||
| val outputs = newPlan.output.zip(p.output) | ||
| // the original Union may produce different output attributes than the new one so we alias | ||
| // them if needed | ||
| if (outputs.forall { case (newAttr, oldAttr) => newAttr.exprId == oldAttr.exprId }) { | ||
| newPlan | ||
| } else { | ||
| val outputAliases = outputs.map { case (newAttr, oldAttr) => | ||
| Alias(newAttr, oldAttr.name)(oldAttr.exprId) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is introducing a new
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, we can lose it in some cases. Fixed in cea0a48 |
||
| } | ||
| Project(outputAliases, newPlan) | ||
| } | ||
| } | ||
|
|
||
| // Joins on empty LocalRelations generated from streaming sources are not eliminated | ||
| // as stateful streaming joins need to perform other state management operations other than | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,7 @@ class PropagateEmptyRelationSuite extends PlanTest { | |
|
|
||
| val testRelation1 = LocalRelation.fromExternalRows(Seq('a.int), data = Seq(Row(1))) | ||
| val testRelation2 = LocalRelation.fromExternalRows(Seq('b.int), data = Seq(Row(1))) | ||
| val testRelation3 = LocalRelation.fromExternalRows(Seq('c.int.notNull), data = Seq(Row(1))) | ||
|
|
||
| test("propagate empty relation through Union") { | ||
| val query = testRelation1 | ||
|
|
@@ -67,6 +68,34 @@ class PropagateEmptyRelationSuite extends PlanTest { | |
| comparePlans(optimized, correctAnswer) | ||
| } | ||
|
|
||
| test("SPARK-32241: remove empty relation children from Union") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a test to check the nullability update?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A simple test case like here: b454cd7 is ok? Or you want me to test that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test case does not apply the UpdateAttributeNullability rules in Optimizer. Do you need to add a test case? |
||
| val query = testRelation1.union(testRelation2.where(false)) | ||
| val optimized = Optimize.execute(query.analyze) | ||
| val correctAnswer = testRelation1 | ||
| comparePlans(optimized, correctAnswer) | ||
|
|
||
| val query2 = testRelation1.where(false).union(testRelation2) | ||
| val optimized2 = Optimize.execute(query2.analyze) | ||
| val correctAnswer2 = testRelation2.select('b.as('a)).analyze | ||
| comparePlans(optimized2, correctAnswer2) | ||
|
|
||
| val query3 = testRelation1.union(testRelation2.where(false)).union(testRelation3) | ||
| val optimized3 = Optimize.execute(query3.analyze) | ||
| val correctAnswer3 = testRelation1.union(testRelation3) | ||
| comparePlans(optimized3, correctAnswer3) | ||
|
|
||
| val query4 = testRelation1.where(false).union(testRelation2).union(testRelation3) | ||
| val optimized4 = Optimize.execute(query4.analyze) | ||
| val correctAnswer4 = testRelation2.union(testRelation3).select('b.as('a)).analyze | ||
| comparePlans(optimized4, correctAnswer4) | ||
|
|
||
| // Nullability can change from nullable to non-nullable | ||
| val query5 = testRelation1.where(false).union(testRelation3).select('a.as('x)).analyze | ||
| val optimized5 = Optimize.execute(query5.analyze) | ||
| assert(query5.output.head.nullable, "Original output should be nullable") | ||
| assert(!optimized5.output.head.nullable, "The new output should be non-nullable") | ||
| } | ||
|
|
||
| test("propagate empty relation through Join") { | ||
| // Testcases are tuples of (left predicate, right predicate, joinType, correct answer) | ||
| // Note that `None` is used to compare with OptimizeWithoutPropagateEmptyRelation. | ||
|
|
||
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.
Can you leave a comment above explaining why we run
UpdateAttributeNullabilityhere?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.
Sure, added in the latest commit.