[SPARK-26735][SQL] Verify plan integrity for special expressions #23658

rednaxelafx · 2019-01-26T00:42:18Z

What changes were proposed in this pull request?

Add verification of plan integrity with regards to special expressions being hosted only in supported operators. Specifically:

AggregateExpression: should only be hosted in Aggregate, or indirectly in Window
WindowExpression: should only be hosted in Window
Generator: should only be hosted in Generate

This will help us catch errors in future optimizer rules that incorrectly hoist special expression out of their supported operator.

TODO: This PR actually caught a bug in the analyzer in the test case SPARK-23957 Remove redundant sort from subquery plan(scalar subquery) in SubquerySuite, where a max() aggregate function is hosted in a Sort operator in the analyzed plan, which is invalid. That test case is disabled in this PR.
SPARK-26741 has been opened to track the fix in the analyzer.

How was this patch tested?

Added new test case in OptimizerStructuralIntegrityCheckerSuite

cloud-fan · 2019-01-26T03:28:29Z

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

nit: use find?

cloud-fan · 2019-01-26T03:33:37Z

I like this check! It can save us a lot of time when debugging!

cloud-fan · 2019-01-26T03:35:51Z

LGTM except a code style comment.

SparkQA · 2019-01-26T03:51:12Z

Test build #101699 has finished for PR 23658 at commit cf3f288.

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

…wExpression and Generator should only be hosted in corresponding operators

viirya · 2019-01-26T08:27:20Z

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

We have another check now. We should update the doc here.

Thanks! Updated

rednaxelafx · 2019-01-26T09:04:39Z

Thanks for your reviews, @cloud-fan and @viirya !

I've updated the PR addressing your comments.

This PR has actually caught a genuine bug in the analyzer in one of the test cases:
For this query in SubquerySuite,

SELECT *
FROM   t1
WHERE  c1 = (SELECT   max(t2.c1)
             FROM     t2
             GROUP BY t2.c1
             HAVING   count(*) >= 1
             ORDER BY max(t2.c1))

The analyzer resolves it into:

== Analyzed Logical Plan ==
c1: int, c2: int
Project [c1#51, c2#52]
+- Filter (c1#51 = scalar-subquery#109 [])
   :  +- Project [max(c1)#137]
   :     +- Sort [max(c1#28) ASC NULLS FIRST], true                // NOTE HERE!!
   :        +- Project [max(c1)#137, c1#28]
   :           +- Filter (count(1)#139L >= cast(1 as bigint))
   :              +- Aggregate [c1#28], [max(c1#28) AS max(c1)#137, count(1) AS count(1)#139L, c1#28]
   :                 +- SubqueryAlias `t2`
   :                    +- Project [_1#23 AS c1#28, _2#24 AS c2#29]
   :                       +- LocalRelation [_1#23, _2#24]
   +- SubqueryAlias `t1`
      +- Project [_1#46 AS c1#51, _2#47 AS c2#52]
         +- LocalRelation [_1#46, _2#47]

... where Sort [max(c1#28) ASC NULLS FIRST], true is an example of a Sort operator hosting an aggregate expression max, incorrectly.

It's somewhat tedious to fix because we need to tweak the order a bit. Working on it.

SparkQA · 2019-01-26T11:02:00Z

Test build #101712 has finished for PR 23658 at commit c7ccc42.

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

gatorsmile · 2019-01-26T17:26:30Z

...scala/org/apache/spark/sql/catalyst/optimizer/OptimizerStructuralIntegrityCheckerSuite.scala

        val newAttr = UnresolvedAttribute("unresolvedAttr")
        Project(projectList ++ Seq(newAttr), child)
+      case agg @ Aggregate(Nil, aggregateExpressions, child) =>
+        Project(aggregateExpressions, child)


Add a comment here to explain Project is unable to handle Aggregate expressions.

gatorsmile · 2019-01-26T17:27:11Z

Please open a JIRA and make the bug fix as a TO-DO item. Thanks!

…fter fixing SPARK-26741

SparkQA · 2019-01-26T23:02:06Z

Test build #101718 has finished for PR 23658 at commit 4c34e98.

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

SparkQA · 2019-01-27T05:12:50Z

Test build #101721 has finished for PR 23658 at commit c1d38dd.

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

gatorsmile · 2019-01-27T06:25:33Z

LGTM

Thanks! Merged to master.

## What changes were proposed in this pull request? Add verification of plan integrity with regards to special expressions being hosted only in supported operators. Specifically: - `AggregateExpression`: should only be hosted in `Aggregate`, or indirectly in `Window` - `WindowExpression`: should only be hosted in `Window` - `Generator`: should only be hosted in `Generate` This will help us catch errors in future optimizer rules that incorrectly hoist special expression out of their supported operator. TODO: This PR actually caught a bug in the analyzer in the test case `SPARK-23957 Remove redundant sort from subquery plan(scalar subquery)` in `SubquerySuite`, where a `max()` aggregate function is hosted in a `Sort` operator in the analyzed plan, which is invalid. That test case is disabled in this PR. SPARK-26741 has been opened to track the fix in the analyzer. ## How was this patch tested? Added new test case in `OptimizerStructuralIntegrityCheckerSuite` Closes apache#23658 from rednaxelafx/plan-integrity. Authored-by: Kris Mok <[email protected]> Signed-off-by: gatorsmile <[email protected]>

cloud-fan reviewed Jan 26, 2019

View reviewed changes

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

Copy link

Contributor

cloud-fan Jan 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use find?

verify plan integrity: special expressions AggregateExpression, Windo…

f06e839

…wExpression and Generator should only be hosted in corresponding operators

viirya reviewed Jan 26, 2019

View reviewed changes

Address comments

c7ccc42

rednaxelafx force-pushed the plan-integrity branch from cf3f288 to c7ccc42 Compare January 26, 2019 09:00

gatorsmile reviewed Jan 26, 2019

View reviewed changes

Disable failing test case in SubquerySuite. It should be re-enabled a…

4c34e98

…fter fixing SPARK-26741

Address Xiao's comment and fix test

c1d38dd

asfgit closed this in 860336d Jan 27, 2019

cloud-fan mentioned this pull request Mar 26, 2019

[SPARK-27255][SQL] Report error when illegal expressions are hosted by a plan operator. #24209

Closed

[SPARK-26735][SQL] Verify plan integrity for special expressions #23658

[SPARK-26735][SQL] Verify plan integrity for special expressions #23658

Uh oh!

Conversation

rednaxelafx commented Jan 26, 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!

cloud-fan Jan 26, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jan 26, 2019

Uh oh!

cloud-fan commented Jan 26, 2019

Uh oh!

SparkQA commented Jan 26, 2019

Uh oh!

viirya Jan 26, 2019

Choose a reason for hiding this comment

Uh oh!

rednaxelafx Jan 26, 2019

Choose a reason for hiding this comment

Uh oh!

rednaxelafx commented Jan 26, 2019

Uh oh!

SparkQA commented Jan 26, 2019

Uh oh!

gatorsmile Jan 26, 2019

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Jan 26, 2019

Uh oh!

SparkQA commented Jan 26, 2019

Uh oh!

SparkQA commented Jan 27, 2019

Uh oh!

gatorsmile commented Jan 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rednaxelafx commented Jan 26, 2019 •

edited

Loading