[SPARK-32241][SQL] Remove empty children of union #29053

peter-toth · 2020-07-09T12:58:45Z

What changes were proposed in this pull request?

This PR removes the empty child relations of a Union.

E.g. the query SELECT c FROM t UNION ALL SELECT c FROM t WHERE false has the following plan before this PR:

== Physical Plan ==
Union
:- *(1) Project [value#219 AS c#222]
:  +- *(1) LocalTableScan [value#219]
+- LocalTableScan <empty>, [c#224]

and after this PR:

== Physical Plan ==
*(1) Project [value#219 AS c#222]
+- *(1) LocalTableScan [value#219]

Why are the changes needed?

To have a simpler plan.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added new UTs.

SparkQA · 2020-07-09T13:06:27Z

Test build #125472 has started for PR 29053 at commit 71b28fd.

dongjoon-hyun · 2020-07-09T17:41:59Z

...yst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/PropagateEmptyRelationSuite.scala

    comparePlans(optimized, correctAnswer)
  }

+  test("remove empty relation children from Union") {


Although this is an improvement, shall we add SPARK-32241: ?

Fixed, thanks.

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PropagateEmptyRelation.scala

dongjoon-hyun · 2020-07-09T17:44:52Z

Also, cc @cloud-fan

maropu · 2020-07-10T01:20:55Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PropagateEmptyRelation.scala

+      if (newChildren.isEmpty) {
+        empty(p)
+      } else {
+        val newFirstChild = if (newChildren.head eq children.head) {


I think you can use sameOutput like this;

if (newChildren.isEmpty) { empty(p) } else { val newPlan = if (newChildren.size > 1) Union(newChildren) else newChildren.head // Describes why we need this handling if (newPlan.sameOutput(p)) { newPlan } else { val outputAliases = newPlan.output.zip(p.output).map { case (newAttr, outputName) => Alias(newAttr, outputName.name)(newAttr.exprId) } Project(outputAliases, newPlan) } }

Seems to be a good idea, but sameOutput uses semanticEquals that fails if an expression is non-deterministic. How about just comparing the expression ids like this?

if (newChildren.isEmpty) { empty(p) } else { 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) } Project(outputAliases, newPlan) } }

I've changed it to my version in the latest commit. @maropu please let me know if yours with sameOutput would be better.

Yea, your version looks okay.

maropu · 2020-07-10T01:21:12Z

also cc: @viirya

HyukjinKwon · 2020-07-10T02:47:39Z

also @maryannxue :-)

viirya · 2020-07-10T06:13:52Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PropagateEmptyRelation.scala

+          val pl = children.head.output.zip(newChildren.head.output).map {
+            case (oa, na) => Alias(na, oa.name)(oa.exprId)
+          }
+          Project(pl, newChildren.head)
+        }


Looks like this may change output nullability of original Union?

but it's corrected?

This project looks correct. Just the new Union's output nullability might change.

Maybe a stupid question, but is that an issue? It can change in one direction, from nullable to non-nullable.

It should not be an issue. But the nullability updates may not be effective unless we run the rule UpdateAttributeNullability

Ah ok, I see. So shall we run UpdateAttributeNullability after PropagateEmptyRelation in the Optimizer?

currently UpdateAttributeNullability is put in a Once batch. We need to evaluate if it's OK (idempotent and cost-efficient) to run it in a fix-point batch.

It looks like it is idempotent and doesn't do costly things just a simple bottom-up traversal on nodes and one expression traversal per node.

I've added UpdateAttributeNullability after PropagateEmptyRelation in the latest commit.

cloud-fan · 2020-07-10T13:41:31Z

...yst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/PropagateEmptyRelationSuite.scala

    comparePlans(optimized, correctAnswer)
  }

+  test("SPARK-32241: remove empty relation children from Union") {


can we add a test to check the nullability update?

A simple test case like here: b454cd7 is ok? Or you want me to test that UpdateAttributeNullability kicks in and fixes some nullability after PropagateEmptyRelation?

This test case does not apply the UpdateAttributeNullability rules in Optimizer. Do you need to add a test case?

SparkQA · 2020-07-10T14:55:03Z

Test build #125610 has finished for PR 29053 at commit f9c0a3d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-10T15:26:46Z

Test build #125623 has started for PR 29053 at commit b454cd7.

viirya · 2020-07-10T16:04:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

+      PropagateEmptyRelation,
+      UpdateAttributeNullability) ::


Can you leave a comment above explaining why we run UpdateAttributeNullability here?

Sure, added in the latest commit.

SparkQA · 2020-07-11T00:21:43Z

Test build #125630 has finished for PR 29053 at commit e10fe02.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-07-12T16:06:16Z

Retest this please.

SparkQA · 2020-07-12T21:24:08Z

Test build #125720 has finished for PR 29053 at commit e10fe02.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-07-12T22:52:58Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PropagateEmptyRelation.scala

+          newPlan
+        } else {
+          val outputAliases = outputs.map { case (newAttr, oldAttr) =>
+            Alias(newAttr, oldAttr.name)(oldAttr.exprId)


Since this is introducing a new Alias, I'm wondering if we need to worry about explicitMetadata in this case.

You are right, we can lose it in some cases. Fixed in cea0a48

SparkQA · 2020-07-13T15:22:07Z

Test build #125767 has finished for PR 29053 at commit 05a90ea.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-14T04:26:26Z

thanks, merging to master!

peter-toth · 2020-07-14T07:49:55Z

Thanks for the review.

dongjoon-hyun · 2020-07-14T07:53:43Z

Thank you, all!

[SPARK-32241][SQL] Remove empty children of union

71b28fd

probot-autolabeler bot added the SQL label Jul 9, 2020

dongjoon-hyun reviewed Jul 9, 2020

View reviewed changes

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PropagateEmptyRelation.scala Show resolved Hide resolved

maropu reviewed Jul 10, 2020

View reviewed changes

viirya reviewed Jul 10, 2020

View reviewed changes

fix review findings

f9c0a3d

cloud-fan reviewed Jul 10, 2020

View reviewed changes

add nullability test

b454cd7

viirya reviewed Jul 10, 2020

View reviewed changes

add comments

e10fe02

dongjoon-hyun reviewed Jul 12, 2020

View reviewed changes

peter-toth added 2 commits July 13, 2020 12:16

keep metadata

cea0a48

clean up code

05a90ea

cloud-fan closed this in 24be816 Jul 14, 2020

ulysses-you mentioned this pull request Jan 11, 2022

[SPARK-35442][SQL] Support propagate empty relation through aggregate/union #35149

Closed

[SPARK-32241][SQL] Remove empty children of union #29053

[SPARK-32241][SQL] Remove empty children of union #29053

Uh oh!

Conversation

peter-toth commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Jul 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu commented Jul 10, 2020

Uh oh!

HyukjinKwon commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinhai-cloud Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 11, 2020

Uh oh!

dongjoon-hyun commented Jul 12, 2020

Uh oh!

SparkQA commented Jul 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter-toth Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 13, 2020

Uh oh!

peter-toth commented Jul 9, 2020 •

edited

Loading

jinhai-cloud Aug 9, 2023 •

edited

Loading

peter-toth Jul 13, 2020 •

edited

Loading