[SPARK-27246][SQL] Add an assert on invalid Scalar subquery plan with no column #24182

sandeep-katta · 2019-03-22T12:07:53Z

What changes were proposed in this pull request?

This PR proposes to add an assert on ScalarSubquery's dataType because there's a possibility that dataType can be called alone before throwing analysis exception.

This was found while working on SPARK-27088. This change calls treeString for logging purpose, and the specific test "scalar subquery with no column" under AnalysisErrorSuite was being failed with:

Caused by: sbt.ForkMain$ForkError: java.util.NoSuchElementException: next on empty iterator
	...
	at scala.collection.mutable.ArrayOps$ofRef.head(ArrayOps.scala:198)
	at org.apache.spark.sql.catalyst.expressions.ScalarSubquery.dataType(subquery.scala:251)
	at org.apache.spark.sql.catalyst.expressions.Alias.dataType(namedExpressions.scala:163)
        ...
	at org.apache.spark.sql.catalyst.trees.TreeNode.simpleString(TreeNode.scala:465)
        ...
	at org.apache.spark.sql.catalyst.rules.RuleExecutor$PlanChangeLogger.logRule(RuleExecutor.scala:176)
	at org.apache.spark.sql.catalyst.rules.RuleExecutor.$anonfun$execute$2(RuleExecutor.scala:116)
	...

The reason is that treeString for logging happened to call dataType on ScalarSubquery but one test has empty column plan. So, it happened to throw NoSuchElementException before checking analysis.

How was this patch tested?

Manually tested.

ScalarSubquery(LocalRelation()).treeString

An exception or error caused a run to abort: assertion failed: Scala subquery should have only one column 
java.lang.AssertionError: assertion failed: Scala subquery should have only one column
	at scala.Predef$.assert(Predef.scala:223)
	at org.apache.spark.sql.catalyst.expressions.ScalarSubquery.dataType(subquery.scala:252)
	at org.apache.spark.sql.catalyst.analysis.AnalysisErrorSuite.<init>(AnalysisErrorSuite.scala:116)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at java.lang.Class.newInstance(Class.java:442)
	at org.scalatest.tools.Runner$.genSuiteConfig(Runner.scala:1428)
	at org.scalatest.tools.Runner$.$anonfun$doRunRunRunDaDoRunRun$8(Runner.scala:1236)
	at scala.collection.immutable.List.map(List.scala:286)
	at org.scalatest.tools.Runner$.doRunRunRunDaDoRunRun(Runner.scala:1235)

sandeep-katta · 2019-03-22T12:11:36Z

cc @maropu

this PR is required for 24136

maropu · 2019-03-22T12:46:14Z

What's the relationship between this and #24136?

sandeep-katta · 2019-03-22T13:30:55Z

"scalar subquery with no column" UT is failing

Reason: treeString is throwing exception , code here. Earlier this statement was in debug log, since debug is disabled this statement never executed in UT, now as per the new changes this treeString API is called so it is throwing exception.

sandeep-katta · 2019-03-22T17:45:51Z

can refer the UT failure report Jenkins Report

HyukjinKwon · 2019-03-25T01:54:18Z

ok to test

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

maropu · 2019-03-25T06:16:29Z

Reason: treeString is throwing exception , code here. Earlier this statement was in debug log, since debug is disabled this statement never executed in UT, now as per the new changes this treeString API is called so it is throwing exception.

If the failure is caused by the change of #24136, we should put this fix there?

sandeep-katta · 2019-03-25T06:20:20Z

Reason: treeString is throwing exception , code here. Earlier this statement was in debug log, since debug is disabled this statement never executed in UT, now as per the new changes this treeString API is called so it is throwing exception.

If the failure is caused by the change of #24136, we should put this fix there?

Failure is not caused by #24136 , it's an existing issue which recovered while fixing

SparkQA · 2019-03-25T06:22:09Z

Test build #103882 has finished for PR 24182 at commit 6f07fb9.

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

maropu · 2019-03-25T06:37:48Z

Ah, I see. Could you add the tests that don't depend on a log level?

dilipbiswal · 2019-03-25T07:04:01Z

@sandeep-katta @maropu
IMHO removing this test and also asserting in the code on a 0 length schema is probably better.
We could replace the test to verify the assert exception + message instead ?
The reason i am thinking this way is "the NullType is not a valid return type of a scalar subquery expression". There is no way to get a user to type in a query that can exhibit this issue, right ?

SparkQA · 2019-03-25T07:05:01Z

Test build #103884 has finished for PR 24182 at commit 20a2aaa.

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

HyukjinKwon · 2019-03-25T07:12:50Z

Yea, adding an assert or throwing an exception should be better. Want to see if that's possible too. So wanted to check which test case is failed for what reason. Can you pull it out @sandeep-katta?

sandeep-katta · 2019-03-25T07:16:19Z

Failed test case Jenkins Report

Caused by: sbt.ForkMain$ForkError: java.util.NoSuchElementException: next on empty iterator
at scala.collection.Iterator$$anon$2.next(Iterator.scala:41)
at scala.collection.Iterator$$anon$2.next(Iterator.scala:39)
at scala.collection.IndexedSeqLike$Elements.next(IndexedSeqLike.scala:63)
at scala.collection.IterableLike.head(IterableLike.scala:109)
at scala.collection.IterableLike.head$(IterableLike.scala:108)
at scala.collection.mutable.ArrayOps$ofRef.scala$collection$IndexedSeqOptimized$$super$head(ArrayOps.scala:198)
at scala.collection.IndexedSeqOptimized.head(IndexedSeqOptimized.scala:129)
at scala.collection.IndexedSeqOptimized.head$(IndexedSeqOptimized.scala:129)
at scala.collection.mutable.ArrayOps$ofRef.head(ArrayOps.scala:198)
at org.apache.spark.sql.catalyst.expressions.ScalarSubquery.dataType(subquery.scala:251)

dilipbiswal · 2019-03-25T07:18:13Z

@HyukjinKwon From one of sandeep's comment i gathered that this is the test (in AnalysisErrorSuite) thats failing :

 errorTest(
    "scalar subquery with no column",
    testRelation.select(ScalarSubquery(LocalRelation()).as('a)),
    "Scalar subquery must return only one column, but got 0" :: Nil)

Here we are purposely setting up an ScalarSubquery expression op top of a relation with 0 length scema. If we run this test normally it passes. But if we turn on plan debugging it fails.
@sandeep-katta Please let me know if i got it right :-)

HyukjinKwon · 2019-03-25T07:21:58Z

Thanks. Removing test and adding an assert looks better to me too.

maropu · 2019-03-25T07:36:01Z

SGTM, too

dilipbiswal · 2019-03-25T08:19:30Z

Looks good to me.
cc @HyukjinKwon @maropu

SparkQA · 2019-03-25T08:52:08Z

Test build #103902 has finished for PR 24182 at commit 726d864.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class SubquerySuite extends SparkFunSuite

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/SubquerySuite.scala

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

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala

HyukjinKwon · 2019-03-25T11:41:01Z

LGTM too, otherwise. For PR description, I have updated by myself but let's keep the PR description detailed and more readable next time.

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/SubquerySuite.scala

SparkQA · 2019-03-25T12:30:42Z

Test build #103901 has finished for PR 24182 at commit 1223fd0.

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

SparkQA · 2019-03-25T13:19:06Z

Test build #103903 has finished for PR 24182 at commit 459914c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class SubquerySuite extends SparkFunSuite

SparkQA · 2019-03-25T16:28:11Z

Test build #103910 has finished for PR 24182 at commit 37ee7d6.

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

HyukjinKwon · 2019-03-26T00:25:34Z

Merged to master.

HyukjinKwon reviewed Mar 25, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/subquery.scala Outdated Show resolved Hide resolved

HyukjinKwon reviewed Mar 25, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/subquery.scala Outdated Show resolved Hide resolved

sandeep-katta changed the title ~~[SPARK-27246][SQL]scalar subquery with no columns throws exception in spark shell~~ [SPARK-27246][SQL]scalar subquery with no column test fails as it is throwing NoSuchElementException instead of AnalysisException Mar 25, 2019

sandeep-katta force-pushed the subqueryissue branch from 20ef5db to 1223fd0 Compare March 25, 2019 08:14

sandeep-katta force-pushed the subqueryissue branch from 13ae066 to 726d864 Compare March 25, 2019 08:48

sandeep-katta force-pushed the subqueryissue branch from 726d864 to 459914c Compare March 25, 2019 09:02

dilipbiswal reviewed Mar 25, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/SubquerySuite.scala Outdated Show resolved Hide resolved

HyukjinKwon reviewed Mar 25, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/subquery.scala Outdated Show resolved Hide resolved

HyukjinKwon changed the title ~~[SPARK-27246][SQL]scalar subquery with no column test fails as it is throwing NoSuchElementException instead of AnalysisException~~ [SPARK-27246][SQL] Add an assert on invalid Scalar subquery plan with no column Mar 25, 2019

HyukjinKwon reviewed Mar 25, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala Outdated Show resolved Hide resolved

sandeep-katta force-pushed the subqueryissue branch from 459914c to cfc2935 Compare March 25, 2019 11:52

maropu reviewed Mar 25, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/SubquerySuite.scala Outdated Show resolved Hide resolved

HyukjinKwon reviewed Mar 25, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/SubquerySuite.scala Outdated Show resolved Hide resolved

Updated as per review comments

37ee7d6

sandeep-katta force-pushed the subqueryissue branch from cfc2935 to 37ee7d6 Compare March 25, 2019 12:11

HyukjinKwon closed this in 0bc030c Mar 26, 2019

[SPARK-27246][SQL] Add an assert on invalid Scalar subquery plan with no column #24182

[SPARK-27246][SQL] Add an assert on invalid Scalar subquery plan with no column #24182

Conversation

sandeep-katta commented Mar 22, 2019 • edited by HyukjinKwon 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!

sandeep-katta commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandeep-katta commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandeep-katta commented Mar 22, 2019

Uh oh!

HyukjinKwon commented Mar 25, 2019

Uh oh!

Uh oh!

Uh oh!

maropu commented Mar 25, 2019

Uh oh!

sandeep-katta commented Mar 25, 2019

Uh oh!

SparkQA commented Mar 25, 2019

Uh oh!

maropu commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dilipbiswal commented Mar 25, 2019

Uh oh!

SparkQA commented Mar 25, 2019

Uh oh!

HyukjinKwon commented Mar 25, 2019

Uh oh!

sandeep-katta commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dilipbiswal commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Mar 25, 2019

Uh oh!

maropu commented Mar 25, 2019

Uh oh!

dilipbiswal commented Mar 25, 2019

Uh oh!

SparkQA commented Mar 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HyukjinKwon commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Mar 25, 2019

Uh oh!

SparkQA commented Mar 25, 2019

Uh oh!

SparkQA commented Mar 25, 2019

Uh oh!

HyukjinKwon commented Mar 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sandeep-katta commented Mar 22, 2019 •

edited by HyukjinKwon

Loading

sandeep-katta commented Mar 22, 2019 •

edited

Loading

maropu commented Mar 22, 2019 •

edited

Loading

sandeep-katta commented Mar 22, 2019 •

edited

Loading

maropu commented Mar 25, 2019 •

edited

Loading

sandeep-katta commented Mar 25, 2019 •

edited

Loading

dilipbiswal commented Mar 25, 2019 •

edited

Loading

HyukjinKwon commented Mar 25, 2019 •

edited

Loading