[SPARK-28143][SQL] Expressions without proper constructors should throw AnalysisException by yaooqinn · Pull Request #24947 · apache/spark

yaooqinn · 2019-06-24T06:15:02Z

What changes were proposed in this pull request?

SQL queries like select 1 where in (), which miss the attribute name should throw AnalysisException with Invalid number of arguments for function in as debug message rather than java.lang.UnsupportedOperationException: empty.init.

This problem I guess is related to this pr - #21226

How was this patch tested?

add ut
Manually

Before

select 1 where in ();
19/06/2413:37:33ERRORSparkSQLDriver:Failed in [select 1 where in ()]
java.lang.UnsupportedOperationException: empty.init

After

Invalid number of arguments for function in. Expected: 2; Found: 0

…is exception

yaooqinn · 2019-06-24T06:15:57Z

cc @cloud-fan

SparkQA · 2019-06-24T07:05:01Z

Test build #106819 has finished for PR 24947 at commit 4af96a1.

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

yaooqinn · 2019-06-24T07:09:39Z

retest this please

SparkQA · 2019-06-24T09:23:03Z

Test build #106823 has finished for PR 24947 at commit 4af96a1.

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

SparkQA · 2019-06-24T17:26:42Z

Test build #106833 has finished for PR 24947 at commit 9ca1990.

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

cloud-fan · 2019-06-25T02:37:07Z

does the problem still exist when we turn on spark.sql.parser.ansi.enabled?

yaooqinn · 2019-06-25T02:42:58Z

@cloud-fan spark.sql.parser.ansi.enabled=true works fine

spark-sql> select 1 where not in ('2019-06-21');
Error in query:
no viable alternative at input 'in'(line 1, pos 19)
== SQL ==
select 1 where not in ('2019-06-21')

cloud-fan · 2019-06-25T03:00:25Z

Then I think it's a parser issue(and is already fixed), we shouldn't hack FunctionRegistry for it.

SparkQA · 2019-06-25T06:22:29Z

Test build #106856 has finished for PR 24947 at commit 15c67d6.

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

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

Test build #106866 has finished for PR 24947 at commit 30ef47e.

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

yaooqinn · 2019-06-25T07:56:23Z

retest this please

SparkQA · 2019-06-25T11:01:17Z

Test build #106874 has finished for PR 24947 at commit 30ef47e.

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

HyukjinKwon · 2019-06-26T00:18:35Z

- val expectedNumberOfParameters = if (validParametersCount.length == 1) {
+ val expectedNumberOfParameters = if (validParametersCount.isEmpty) {
+ constructors.headOption.map(_.getParameterCount).getOrElse(0).toString
+ } else if (validParametersCount.length == 1) {


@yaooqinn, can you show the error message before and after this change, and fix the PR description and title? I think this PR now targets throw a better exception for the expression that has no constructors to call.

srowen · 2019-07-16T18:35:43Z

@yaooqinn I think there are some last comments to address?

yaooqinn · 2019-07-17T06:12:21Z

@srowen I have updated the pr description and title as last comment required. If anything I misread, please correct me. Thanks very much.

srowen · 2019-07-17T12:44:29Z

@HyukjinKwon is that OK by you?

SparkQA · 2019-07-21T18:03:56Z

Test build #4826 has finished for PR 24947 at commit 30ef47e.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

gatorsmile · 2019-07-22T03:20:39Z

 .map(_.getParameterCount).distinct.sorted
- val expectedNumberOfParameters = if (validParametersCount.length == 1) {
+ val expectedNumberOfParameters = if (validParametersCount.isEmpty) {
+ constructors.headOption.map(_.getParameterCount).getOrElse(0).toString


Using headIption is hacky. We should follow the same logic what we are doing here.

gatorsmile · 2019-07-22T03:25:03Z

@@ -583,7 +583,9 @@ object FunctionRegistry {
 val validParametersCount = constructors
 .filter(_.getParameterTypes.forall(_ == classOf[Expression]))


If we remove this line, what is the outcome?
In this PR, the logic is to add a special case. When unable to find a constructor whose parameters are all Expressions, we use the remaining constructors. [Note, headOption means we only choose the first one] Could you please check whether we should list all of them in the error messages?

If we remove this line, it works fine. I did this in the first commit of the pr, discussed with HyukjinKwon here

No, It doesn't work fine.
We should only recognise the constructor with expressions because those are only able to be used in SQL. Otherwise, it shows incorrect information about possible argument combination which partially reverts #21226.

The problem is that we have function registered in but that's unable/not supposed to to call via in(...) currently. Possibly there might be more cases like this.

SparkQA · 2019-07-26T07:05:02Z

Test build #108197 has finished for PR 24947 at commit a0363de.

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

yaooqinn · 2019-07-26T16:00:42Z

retest this please

SparkQA · 2019-07-26T21:19:33Z

Test build #108226 has finished for PR 24947 at commit a0363de.

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

HyukjinKwon · 2019-08-22T12:23:35Z

Seems we're blocked by #24947 (comment), shall we close this and investigate if we can remove those functions from function registry? That way will cleanly fix this issue.

[SPARK-28143][SQL]IN expression missing attribute should throw analys…
4af96a1
…is exception

HyukjinKwon reviewed Jun 24, 2019
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala

bring back filter logic and and an or condition
9ca1990

HyukjinKwon reviewed Jun 24, 2019
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Outdated

HyukjinKwon reviewed Jun 24, 2019
View reviewed changes

Comment threadsql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala Outdated

HyukjinKwon reviewed Jun 24, 2019
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Outdated

dongjoon-hyun added the SQL label Jun 24, 2019

if valid paras count == 0
15c67d6

HyukjinKwon reviewed Jun 25, 2019
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Outdated

cloud-fan reviewed Jun 25, 2019
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Outdated

unified err msg
30ef47e

HyukjinKwon reviewed Jun 26, 2019
View reviewed changes

yaooqinn changed the title ~~[SPARK-28143][SQL]IN expression missing attribute should throw analysis exception~~[SPARK-28143][SQL]Expressions without proper constructors should throw AnalysisExceptionJun 26, 2019

dongjoon-hyun changed the title ~~[SPARK-28143][SQL]Expressions without proper constructors should throw AnalysisException~~[SPARK-28143][SQL] Expressions without proper constructors should throw AnalysisExceptionJul 6, 2019

gatorsmile reviewed Jul 22, 2019
View reviewed changes

add some hints when the registered function is defined wrong or misused
a0363de

HyukjinKwon closed this Sep 18, 2019

HyukjinKwon mentioned this pull request Oct 8, 2019
[SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case' #26053
Closed

		@@ -583,7 +583,9 @@ object FunctionRegistry {
		val validParametersCount = constructors
		.filter(_.getParameterTypes.forall(_ == classOf[Expression]))

Uh oh!

Conversation

yaooqinn commented Jun 24, 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!

yaooqinn commented Jun 24, 2019

Uh oh!

SparkQA commented Jun 24, 2019

Uh oh!

yaooqinn commented Jun 24, 2019

Uh oh!

Uh oh!

SparkQA commented Jun 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jun 24, 2019

Uh oh!

cloud-fan commented Jun 25, 2019

Uh oh!

yaooqinn commented Jun 25, 2019

Uh oh!

cloud-fan commented Jun 25, 2019

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jun 25, 2019

Uh oh!

SparkQA commented Jun 25, 2019

Uh oh!

yaooqinn commented Jun 25, 2019

Uh oh!

SparkQA commented Jun 25, 2019

Uh oh!

HyukjinKwonJun 26, 2019

Choose a reason for hiding this comment

Uh oh!

srowen commented Jul 16, 2019

Uh oh!

yaooqinn commented Jul 17, 2019

Uh oh!

srowen commented Jul 17, 2019

Uh oh!

SparkQA commented Jul 21, 2019

Uh oh!

gatorsmileJul 22, 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!

gatorsmileJul 22, 2019

Choose a reason for hiding this comment

Uh oh!

yaooqinnJul 22, 2019

Choose a reason for hiding this comment

Uh oh!

HyukjinKwonJul 26, 2019

Choose a reason for hiding this comment

Uh oh!

HyukjinKwonJul 26, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 26, 2019

Uh oh!

yaooqinn commented Jul 26, 2019

Uh oh!

SparkQA commented Jul 26, 2019

Uh oh!

HyukjinKwon commented Aug 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yaooqinn commented Jun 24, 2019•
edited by HyukjinKwon
Loading

gatorsmileJul 22, 2019•
edited
Loading