[SPARK-20417][SQL] Move subquery error handling to checkAnalysis from Analyzer #17713

dilipbiswal · 2017-04-21T08:20:37Z

What changes were proposed in this pull request?

Currently we do a lot of validations for subquery in the Analyzer. We should move them to CheckAnalysis which is the framework to catch and report Analysis errors. This was mentioned as a review comment in SPARK-18874.

How was this patch tested?

Exists tests + A few tests added to SQLQueryTestSuite.

SparkQA · 2017-04-21T10:32:46Z

Test build #76023 has finished for PR 17713 at commit 17eebd4.

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

dilipbiswal · 2017-04-21T17:51:44Z

cc @hvanhovell @gatorsmile

SparkQA · 2017-04-21T22:12:52Z

Test build #76043 has finished for PR 17713 at commit 39e8cf7.

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

gatorsmile · 2017-04-22T07:20:52Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Just copy it from Analyzer.scala? Please leave some comments for saving the time of reviewers. Thanks!

@gatorsmile Thanks for pointing this. Yes.. Its basically copied from Analyzer.scala.

gatorsmile · 2017-04-22T07:22:07Z

sql/core/src/test/resources/sql-tests/inputs/subquery/negative-cases/subq-input-typecheck.sql

These negative test cases issue the exactly same errors? I mean, before this PR and after this PR.

@gatorsmile Actually there are a few minor difference in the errors that are issued. Thats the reason i decided to create the new tests for them to capture these new errors. Here are the difference.

In Subquery.
When the the number of columns in left hand and right hand sides of in subquery didn't match we raised the error from here in Analyzer. This handled both In and Scalar subquery.
Now we capture it in checkInputTypes here and return a slightly clearer error message to the user.

Scalar Subquery
The number of output column > 1 condition was being handled in two places, one in Analyzer here that dealt with correlated scalar subquery and one in checkAnalysis here dealt with non-correlated scalar subquery. This is now consolidated at a single place here.

dilipbiswal · 2017-04-22T21:35:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

note to reviewers: This function basically refactors the validation logic for subquery expressions from checkAnalysis. This is the entry point function to do all the validation for subquery is is called from checkAnalysis().

dilipbiswal · 2017-05-01T18:08:57Z

ping @gatorsmile @hvanhovell

gatorsmile · 2017-05-06T06:07:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

expressions is not needed, right?

@gatorsmile Yeah.. its not needed. Fixed. Thanks.

gatorsmile · 2017-05-06T06:24:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

Could we output what are left sides and what are right sides? When we have nested subqueries in a very complex user query, it could be hard for users to locate it.

@gatorsmile I have made changes. Can you please check and let me know if it looks okay to you ? I have also added the subquery expression id for better correlation. Please let me know what you think.

gatorsmile · 2017-05-06T06:24:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

The same here

@gatorsmile Same as above.

gatorsmile · 2017-05-06T06:35:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

Could you update it and make it more clear? Basically, we are resolving the RHS plans in the subquery and populating the children using outer references, right?

@gatorsmile Have made changes. Pl. let me know if it looks ok.

gatorsmile · 2017-05-06T07:00:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

checkAggregate is only for Scalar subqueries. Maybe you can rename it.

@gatorsmile Renamed.

gatorsmile · 2017-05-06T07:02:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

The same here. It is only for Scalar subqueries.

@gatorsmile renamed.

gatorsmile · 2017-05-06T07:08:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

I prefer to making it longer but clearer. inOrExistsSubquery -> inSubqueryOrExistsSubquery

@gatorsmile okay. changed.

gatorsmile · 2017-05-06T07:10:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Predicate sub-queries -> IN/EXISTS predicate sub-queries

@gatorsmile changed.

gatorsmile · 2017-05-06T07:11:39Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

s" -> "

@gatorsmile ok.

gatorsmile · 2017-05-06T07:12:26Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

must be Aggregated -> must be aggregated

@gatorsmile changed.

gatorsmile · 2017-05-06T07:13:01Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Nit: Indent issue.

@gatorsmile fixed.

gatorsmile · 2017-05-06T07:17:18Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Revert it back.

gatorsmile · 2017-05-06T07:20:54Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Previously, this checking logics in this function is invoked only when sub is resolved. Do we still need it?

@gatorsmile Yeah.. i had thought about it as well. I have moved this now after checkAnalysis on the sub plan.

gatorsmile · 2017-05-06T07:25:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Nit: foundNonEqualCorrelatedPred : Boolean -> foundNonEqualCorrelatedPred: Boolean

gatorsmile · 2017-05-06T07:33:09Z

It looks pretty good. Just left some comments. Thanks!

dilipbiswal · 2017-05-07T07:52:01Z

@gatorsmile Thanks a lot. Have addressed your comments. Please check when you get a chance.

SparkQA · 2017-05-07T11:57:36Z

Test build #76544 has finished for PR 17713 at commit 8314954.

This patch fails from timeout after a configured wait of `250m`.
This patch merges cleanly.
This patch adds no public classes.

dilipbiswal · 2017-05-07T16:32:54Z

retest this please

SparkQA · 2017-05-07T18:52:34Z

Test build #76547 has finished for PR 17713 at commit 8314954.

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

gatorsmile · 2017-05-07T21:12:31Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

Subquery expression id: #${exprId.id} does not help to the end users, I think

@gatorsmile OK.. since we will have the plan information available for these analysis error, i was thinking it was possible to co-relate the error with the originating subquery expression. Let me remove it, given you think it may not be useful to the end-users as they may not be familiar with the system generated expression id.

gatorsmile · 2017-05-07T21:15:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

How about cleanQueryInScalarSubquery?

@gatorsmile OK.

gatorsmile · 2017-05-07T21:16:56Z

LGTM except minor comments.

cc @hvanhovell

SparkQA · 2017-05-08T00:53:24Z

Test build #76553 has finished for PR 17713 at commit 3c4f38e.

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

gatorsmile · 2017-05-10T06:52:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

See the PR: #17930

It should be moved earlier.

@gatorsmile Thanks for letting me know. I will change.

SparkQA · 2017-05-11T01:56:10Z

Test build #76761 has finished for PR 17713 at commit 9ef29c7.

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

dilipbiswal · 2017-05-11T07:13:10Z

retest this please

SparkQA · 2017-05-11T09:31:57Z

Test build #76789 has finished for PR 17713 at commit 9ef29c7.

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

dilipbiswal · 2017-05-16T20:44:30Z

ping @hvanhovell

gatorsmile · 2017-06-22T00:35:13Z

@dilipbiswal Could you resolve the conflict?

…yzer.

dilipbiswal · 2017-06-22T01:31:29Z

@gatorsmile Just rebased. Thanks !!

SparkQA · 2017-06-22T03:51:14Z

Test build #78418 has finished for PR 17713 at commit c2a7555.

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

gatorsmile · 2017-06-23T06:42:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

-               |[${valExprs.map(_.dataType.catalogString).mkString(", ")}].
-               |Right side:
-               |[${sub.output.map(_.dataType.catalogString).mkString(", ")}].
+               |The number of columns the left hand side of an IN subquery does not match the


The number of columns the left -> The number of columns in the left

gatorsmile · 2017-06-23T06:49:02Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

  override def checkInputDataTypes(): TypeCheckResult = {
    list match {
-      case ListQuery(sub, _, _) :: Nil =>
+      case ListQuery(sub, _, exprId) :: Nil =>


Revert it back?

gatorsmile · 2017-06-23T06:57:45Z

LGTM except two minor comments.

SparkQA · 2017-06-23T09:50:21Z

Test build #78512 has finished for PR 17713 at commit b91aea2.

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

gatorsmile · 2017-06-23T18:03:10Z

Thanks! Merging to master.

dilipbiswal · 2017-06-23T18:19:27Z

@gatorsmile Thank you very much !!

… Analyzer ## What changes were proposed in this pull request? Currently we do a lot of validations for subquery in the Analyzer. We should move them to CheckAnalysis which is the framework to catch and report Analysis errors. This was mentioned as a review comment in SPARK-18874. ## How was this patch tested? Exists tests + A few tests added to SQLQueryTestSuite. Author: Dilip Biswal <[email protected]> Closes apache#17713 from dilipbiswal/subquery_checkanalysis.

gatorsmile reviewed Apr 22, 2017

View reviewed changes

dilipbiswal commented Apr 22, 2017

View reviewed changes

gatorsmile reviewed May 6, 2017

View reviewed changes

gatorsmile reviewed May 7, 2017

View reviewed changes

gatorsmile reviewed May 10, 2017

View reviewed changes

dilipbiswal force-pushed the subquery_checkanalysis branch from 3c4f38e to 9ef29c7 Compare May 11, 2017 00:29

dilipbiswal added 4 commits June 21, 2017 17:37

[SPARK-20417] Move subquery error handling to checkAnalysis from Anal…

08a112e

…yzer.

Remove unused import

9ee2b88

Code review comments

6eb406e

review comments

c2a7555

dilipbiswal force-pushed the subquery_checkanalysis branch from 9ef29c7 to c2a7555 Compare June 22, 2017 01:30

gatorsmile reviewed Jun 23, 2017

View reviewed changes

review comments

b91aea2

asfgit closed this in 13c2a4f Jun 23, 2017

dilipbiswal mentioned this pull request Mar 26, 2019

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

Closed

[SPARK-20417][SQL] Move subquery error handling to checkAnalysis from Analyzer #17713

[SPARK-20417][SQL] Move subquery error handling to checkAnalysis from Analyzer #17713

Uh oh!

Conversation

dilipbiswal commented Apr 21, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 21, 2017

Uh oh!

dilipbiswal commented Apr 21, 2017

Uh oh!

SparkQA commented Apr 21, 2017

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 May 1, 2017

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!

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!

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!

gatorsmile commented May 6, 2017

Uh oh!