[SPARK-26390][SQL] ColumnPruning rule should only do column pruning #23343

cloud-fan · 2018-12-18T07:24:58Z

What changes were proposed in this pull request?

This is a small clean up.

By design catalyst rules should be orthogonal: each rule should have its own responsibility. However, the ColumnPruning rule does not only do column pruning, but also remove no-op project and window.

This PR updates the RemoveRedundantProject rule to remove no-op window as well, and clean up the ColumnPruning rule to only do column pruning.

How was this patch tested?

existing tests

cloud-fan · 2018-12-18T07:25:43Z

cc @gatorsmile @dongjoon-hyun @viirya

SparkQA · 2018-12-18T08:05:01Z

Test build #100274 has finished for PR 23343 at commit aad89d3.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2018-12-18T09:15:35Z

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

@@ -93,7 +93,7 @@ abstract class Optimizer(sessionCatalog: SessionCatalog)
        RewriteCorrelatedScalarSubquery,
        EliminateSerialization,
        RemoveRedundantAliases,
-        RemoveRedundantProject,
+        RemoveNoopOperators,


nit: RemoveUselessOperators?

I think Noop is fine too. It's no-op, right :-)?

yes it is fine too, I preferred Useless because they are actually doing something, so they introduce a useless overhead, anyway not a big deal

mgaido91 · 2018-12-18T09:18:30Z

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

    // Can't prune the columns on LeafNode
    case p @ Project(_, _: LeafNode) => p

    // for all other logical plans that inherits the output from it's children
-    case p @ Project(_, child) =>
+    case p @ Project(_, child) if !child.isInstanceOf[Project] =>


in case the child is a project, shall we anyway update it with c.output.filter(allReferences.contains)? I mean can we instead update the prunedChild method to check if c is a Project and behave accordingly?

We already handled project over project at L542

yes, I see, makes sense, thanks.

This requires a code comment. I believe the others will ask the same q when we read the problem again.

cloud-fan · 2018-12-18T10:59:01Z

retest this please

viirya · 2018-12-18T13:34:45Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/ColumnPruningSuite.scala

@@ -34,6 +34,7 @@ class ColumnPruningSuite extends PlanTest {
    val batches = Batch("Column pruning", FixedPoint(100),
      PushDownPredicate,
      ColumnPruning,
+      RemoveNoopOperators,


Is this added to remove top Project('b :: Nil ...)? Without that, this test can be unchanged?

without this, a lot more tests need to be updated...

ah, I see. :)

viirya · 2018-12-18T13:35:58Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/CombiningLimitsSuite.scala

-        ColumnPruning) ::
+      Batch("Column Pruning", FixedPoint(100),
+        ColumnPruning,
+        RemoveNoopOperators) ::


nit: Looks not precise to have RemoveNoopOperators in Column Pruning batch, but it is fine as this is just test.

SparkQA · 2018-12-18T14:54:23Z

Test build #100284 has finished for PR 23343 at commit aad89d3.

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

gatorsmile · 2018-12-19T00:18:31Z

In general, we normally do not encourage the code refactoring in optimizer rules. Any change could introduce a perf regression in query planning. For this PR, I think it is safe although it could slightly increase the number of runs in the big optimizer batch.

LGTM. Please update the comment.

dongjoon-hyun

+1, LGTM.

dongjoon-hyun · 2018-12-19T04:54:29Z

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

+    case p @ Project(_, child) if child.sameOutput(p) => child
+
+    // Eliminate no-op Window
+    case w: Window if w.windowExpressions.isEmpty => w.child
  }
 }


@cloud-fan . Is this too small to move out as a separate file during this refactoring?

It's small and it has been here for a while. We can move many rules here to separated files in another PR.

dongjoon-hyun · 2018-12-19T04:54:51Z

cc @dbtsai

cloud-fan · 2018-12-19T11:33:09Z

retest this please

dongjoon-hyun · 2018-12-19T17:40:36Z

The final commit is only about adding a comment. I'll merge this to master. Thank you all.

dbtsai · 2018-12-19T19:02:55Z

@dongjoon-hyun thanks for pinging me. I'm working on ColumnPruning to add support of pruning nested fields. Since Optimizer.scala is such a huge file now, I'll go ahead to submit a PR to move ColumnPruning out as a separate file. Thanks!

gatorsmile · 2018-12-20T18:57:56Z

@dbtsai It will make us hard to track the change history. Let us avoid moving the existing rules to new files.

dbtsai · 2018-12-20T23:45:54Z

@gatorsmile for example, we were refactoring ReplaceNullWithFalseInPredicate out from expressions.scala as it grows too big, #23139.

I'm wondering when we should or should not do such refactoring. I create a PR #23359 so we can discuss over there.

gatorsmile · 2018-12-21T00:16:35Z

ReplaceNullWithFalseInPredicate is a new rule in 3.0

## What changes were proposed in this pull request? This is a small clean up. By design catalyst rules should be orthogonal: each rule should have its own responsibility. However, the `ColumnPruning` rule does not only do column pruning, but also remove no-op project and window. This PR updates the `RemoveRedundantProject` rule to remove no-op window as well, and clean up the `ColumnPruning` rule to only do column pruning. ## How was this patch tested? existing tests Closes apache#23343 from cloud-fan/column-pruning. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

ColumnPruning rule should only do column pruning

aad89d3

mgaido91 reviewed Dec 18, 2018

View reviewed changes

viirya reviewed Dec 18, 2018

View reviewed changes

viirya approved these changes Dec 18, 2018

View reviewed changes

mgaido91 approved these changes Dec 18, 2018

View reviewed changes

dongjoon-hyun approved these changes Dec 19, 2018

View reviewed changes

dongjoon-hyun reviewed Dec 19, 2018

View reviewed changes

add comment

2b7d930

asfgit closed this in 08f74ad Dec 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-26390][SQL] ColumnPruning rule should only do column pruning #23343

[SPARK-26390][SQL] ColumnPruning rule should only do column pruning #23343

cloud-fan commented Dec 18, 2018

cloud-fan commented Dec 18, 2018

SparkQA commented Dec 18, 2018

mgaido91 Dec 18, 2018

HyukjinKwon Dec 18, 2018

mgaido91 Dec 18, 2018

mgaido91 Dec 18, 2018

cloud-fan Dec 18, 2018

mgaido91 Dec 18, 2018

gatorsmile Dec 19, 2018

cloud-fan commented Dec 18, 2018

viirya Dec 18, 2018

cloud-fan Dec 18, 2018

viirya Dec 18, 2018

viirya Dec 18, 2018

SparkQA commented Dec 18, 2018

gatorsmile commented Dec 19, 2018

dongjoon-hyun left a comment

dongjoon-hyun Dec 19, 2018

cloud-fan Dec 19, 2018

dongjoon-hyun commented Dec 19, 2018

cloud-fan commented Dec 19, 2018

dongjoon-hyun commented Dec 19, 2018

dbtsai commented Dec 19, 2018

gatorsmile commented Dec 20, 2018

dbtsai commented Dec 20, 2018 •

edited

gatorsmile commented Dec 21, 2018

[SPARK-26390][SQL] ColumnPruning rule should only do column pruning #23343

[SPARK-26390][SQL] ColumnPruning rule should only do column pruning #23343

Conversation

cloud-fan commented Dec 18, 2018

What changes were proposed in this pull request?

How was this patch tested?

cloud-fan commented Dec 18, 2018

SparkQA commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 18, 2018

gatorsmile commented Dec 19, 2018

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Dec 19, 2018

cloud-fan commented Dec 19, 2018

dongjoon-hyun commented Dec 19, 2018

dbtsai commented Dec 19, 2018

gatorsmile commented Dec 20, 2018

dbtsai commented Dec 20, 2018 • edited

gatorsmile commented Dec 21, 2018

dbtsai commented Dec 20, 2018 •

edited