[SPARK-27342][SQL] Optimize Limit 0 queries #24271

aayushmaanjain · 2019-04-02T05:38:29Z

What changes were proposed in this pull request?

With this change, unnecessary file scans are avoided in case of Limit 0 queries.

I added a case (rule) to PropagateEmptyRelation to replace GlobalLimit 0 and LocalLimit 0 nodes with an empty LocalRelation. This prunes the subtree under the Limit 0 node and further allows other rules of PropagateEmptyRelation to optimize the Logical Plan - while remaining semantically consistent with the Limit 0 query.

For instance:
Query:
SELECT * FROM table1 INNER JOIN (SELECT * FROM table2 LIMIT 0) AS table2 ON table1.id = table2.id

Optimized Plan without fix:

Join Inner, (id#79 = id#87)
:- Filter isnotnull(id#79)
:  +- Relation[id#79,num1#80] parquet
+- Filter isnotnull(id#87)
   +- GlobalLimit 0
      +- LocalLimit 0
         +- Relation[id#87,num2#88] parquet

Optimized Plan with fix:
LocalRelation <empty>, [id#75, num1#76, id#77, num2#78]

How was this patch tested?

Added unit tests to verify Limit 0 optimization for:

Simple query containing Limit 0
Inner Join, Left Outer Join, Right Outer Join, Full Outer Join queries containing Limit 0 as one of their children
Nested Inner Joins between 3 tables with one of them having a Limit 0 clause.
Intersect query wherein one of the subqueries was a Limit 0 query.

srowen · 2019-04-02T13:44:29Z

Seems reasonable, but @gengliangwang do you have an opinion?

SparkQA · 2019-04-02T18:19:46Z

Test build #4680 has finished for PR 24271 at commit d5fed71.

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

rxin · 2019-04-02T21:36:08Z

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

+    // changes up the Logical Plan.
+    //
+    // Replace Global Limit 0 nodes with empty Local Relation
+    case p @ GlobalLimit(IntegerLiteral(limit), _) if limit == 0 =>


it's better to do a new rule, to make it more modular.

+1, we can also create a new Batch for the new rule, since it requires only one-time transformation.

@rxin @gengliangwang Hi!
We are relying on the other case rules in PropagateEmptyRelation to propagate the effects of substituting Limit 0 with empty LocalRelation through the Logical Plan - such as in case of joins, project, etc.
So, should we handle this case also part of this rule, and then propagation of empty relation happens only as part of this rule? Or do you recommend doing it as a separate rule?

The optimization in this PR is not directly related to "propagate". We can create a separate rule and batch , e.g.

Batch("OptimizeLimitZero", Once, OptimizeLimitZero) :+ Batch("LocalRelation", fixedPoint, ConvertToLocalRelation, PropagateEmptyRelation )

@rxin @gengliangwang @srowen I have made the recommended changes. Please have a look. Thanks!

attilapiros

Just minor stuff and a question. Otherwise LGTM.

attilapiros · 2019-04-03T20:32:21Z

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

+  val testRelation1 = LocalRelation.fromExternalRows(Seq('a.int), data = Seq(Row(1)))
+  val testRelation2 = LocalRelation.fromExternalRows(Seq('b.int), data = Seq(Row(1)))
+
+


super Nit: too much empty lines

attilapiros · 2019-04-03T20:59:23Z

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

+    comparePlans(optimized, correctAnswer)
+  }
+
+  test("Limit 0: Joins") {


You know you can create easily separated test for each join types, like this:

Seq( (Inner, LocalRelation('a.int, 'b.int)), (LeftOuter, Project(Seq('a, Literal(null).cast(IntegerType).as('b)), testRelation1).analyze), (RightOuter, LocalRelation('a.int, 'b.int)), (FullOuter, Project(Seq('a, Literal(null).cast(IntegerType).as('b)), testRelation1).analyze) ).foreach { case (jt, answer) => test(s"Limit 0: for join type $jt") { val query = testRelation1 .join(testRelation2.limit(0), jt, condition = Some('a.attr == 'b.attr)) val optimized = Optimize.execute(query.analyze) val correctAnswer = answer comparePlans(optimized, correctAnswer) } }

So this way the suite execution will contain sth like:

[info] - Limit 0: for join type Inner (30 milliseconds) [info] - Limit 0: for join type LeftOuter (21 milliseconds) [info] - Limit 0: for join type RightOuter (13 milliseconds) [info] - Limit 0: for join type FullOuter (12 milliseconds)

It has only one little advantage if more than one would fail execution would not stop at the first assert.
But your current solution is also very fine.

@attilapiros thanks for the input. I made the required changes.

gengliangwang · 2019-04-04T06:32:20Z

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

    // For example, a query such as Filter(LocalRelation) would go through all the heavy
    // optimizer rules that are triggered when there is a filter
    // (e.g. InferFiltersFromConstraints). If we run this batch earlier, the query becomes just
    // LocalRelation and does not trigger many rules.


The comment should be before the batch LocalRelation early.

@gengliangwang, fixed this.

gengliangwang · 2019-04-04T07:04:13Z

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

+class OptimizeLimitZeroSuite extends PlanTest {
+  object Optimize extends RuleExecutor[LogicalPlan] {
+    val batches =
+      Batch("OptimizeLimitZero", Once,


I think we can have two batches here, just the same as Optimizer.

@gengliangwang Is there a specific reason for having it that way? I modelled the test suite based on PropagateEmptyRelation, wherein also they have all rules as part of the same batch..

gengliangwang

LGTM except my last comment in the test suite.

dilipbiswal · 2019-04-04T08:24:36Z

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

+          .join(testRelation2.limit(0), joinType = jt, condition = Some('a.attr == 'b.attr))
+
+        val optimized = Optimize.execute(query.analyze)
+        val correctAnswer = answer


redundant var ?

dilipbiswal · 2019-04-04T08:25:47Z

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

+
+/**
+ * Replaces GlobalLimit 0 and LocalLimit 0 nodes (subtree) with empty Local Relation, as they don't
+  * return any rows.


fix the alignment here ?

dilipbiswal · 2019-04-04T08:26:32Z

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

+    // changes up the Logical Plan.
+    //
+    // Replace Global Limit 0 nodes with empty Local Relation
+    case gl @ GlobalLimit(IntegerLiteral(limit), _) if limit == 0 =>


case gl @ GlobalLimit(IntegerLiteral(0), _) => ?

@dilipbiswal good catch! Made this change.

dilipbiswal · 2019-04-04T08:26:49Z

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

+    // then the following rule will handle that case as well.
+    //
+    // Replace Local Limit 0 nodes with empty Local Relation
+    case ll @ LocalLimit(IntegerLiteral(limit), _) if limit == 0 =>


same as above..

dilipbiswal · 2019-04-04T20:16:06Z

retest this please

dilipbiswal · 2019-04-04T21:57:14Z

@srowen Hi Sean.. for some reason, its not responding to "retest" ?

srowen · 2019-04-04T22:05:33Z

Jenkins add to whitelist

SparkQA · 2019-04-05T02:36:19Z

Test build #104310 has finished for PR 24271 at commit 1936ec6.

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

dilipbiswal · 2019-04-05T02:38:14Z

LGTM

SparkQA · 2019-04-05T02:43:20Z

Test build #4690 has finished for PR 24271 at commit 1936ec6.

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

gatorsmile

LGTM

Thanks! Merged to master.

aayushmaanjain added 3 commits March 28, 2019 11:11

Add case (rule) to optimize limit 0 queries

df05284

Add unit tests to verify limit 0 optimization

84f944c

Lint fix

d5fed71

rxin reviewed Apr 2, 2019

View reviewed changes

added new rule for Limit 0 optimization

5dd33f8

attilapiros reviewed Apr 3, 2019

View reviewed changes

gengliangwang reviewed Apr 4, 2019

View reviewed changes

PR fixes

ce3e936

gengliangwang reviewed Apr 4, 2019

View reviewed changes

gengliangwang approved these changes Apr 4, 2019

View reviewed changes

dilipbiswal reviewed Apr 4, 2019

View reviewed changes

PR fixes

1936ec6

gatorsmile reviewed Apr 5, 2019

View reviewed changes

gatorsmile changed the title ~~[SPAR-27342][SQL] Optimize Limit 0 queries~~ [SPARK-27342][SQL] Optimize Limit 0 queries Apr 5, 2019

gatorsmile closed this in 04e53d2 Apr 5, 2019

		val testRelation1 = LocalRelation.fromExternalRows(Seq('a.int), data = Seq(Row(1)))
		val testRelation2 = LocalRelation.fromExternalRows(Seq('b.int), data = Seq(Row(1)))

[SPARK-27342][SQL] Optimize Limit 0 queries #24271

[SPARK-27342][SQL] Optimize Limit 0 queries #24271

Uh oh!

Conversation

aayushmaanjain commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen commented Apr 2, 2019

Uh oh!

SparkQA commented Apr 2, 2019

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!

gengliangwang Apr 3, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

attilapiros left a comment

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!

aayushmaanjain Apr 4, 2019 • 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!

gengliangwang left a comment

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!

dilipbiswal commented Apr 4, 2019

Uh oh!

dilipbiswal commented Apr 4, 2019

Uh oh!

srowen commented Apr 4, 2019

Uh oh!

SparkQA commented Apr 5, 2019

Uh oh!

dilipbiswal commented Apr 5, 2019

Uh oh!

SparkQA commented Apr 5, 2019

Uh oh!

gatorsmile left a comment

Choose a reason for hiding this comment

Uh oh!

aayushmaanjain commented Apr 2, 2019 •

edited

Loading

gengliangwang Apr 3, 2019 •

edited

Loading

aayushmaanjain Apr 4, 2019 •

edited

Loading