[SPARK-29026][SQL] Improve error message in `schemaFor` in trait without companion object constructor #25736

mickjermsurawong-stripe · 2019-09-09T21:47:39Z

What changes were proposed in this pull request?

For trait without companion object constructor, currently the method to get constructor parameters constructParams in ScalaReflection will throw exception.

scala.ScalaReflectionException: <none> is not a term
	at scala.reflect.api.Symbols$SymbolApi.asTerm(Symbols.scala:211)
	at scala.reflect.api.Symbols$SymbolApi.asTerm$(Symbols.scala:211)
	at scala.reflect.internal.Symbols$SymbolContextApiImpl.asTerm(Symbols.scala:106)
	at org.apache.spark.sql.catalyst.ScalaReflection.getCompanionConstructor(ScalaReflection.scala:909)
	at org.apache.spark.sql.catalyst.ScalaReflection.constructParams(ScalaReflection.scala:914)
	at org.apache.spark.sql.catalyst.ScalaReflection.constructParams$(ScalaReflection.scala:912)
	at org.apache.spark.sql.catalyst.ScalaReflection$.constructParams(ScalaReflection.scala:47)
	at org.apache.spark.sql.catalyst.ScalaReflection.getConstructorParameters(ScalaReflection.scala:890)
	at org.apache.spark.sql.catalyst.ScalaReflection.getConstructorParameters$(ScalaReflection.scala:886)
	at org.apache.spark.sql.catalyst.ScalaReflection$.getConstructorParameters(ScalaReflection.scala:47)

Instead this PR would throw exception:

Unable to find constructor for type [XXX]. This could happen if [XXX] is an interface or a trait without companion object constructor
UnsupportedOperationException:

In the normal usage of ExpressionEncoder, this can happen if the type is interface extending scala.Product. Also, since this is a protected method, this could have been other arbitrary types without constructor.

Why are the changes needed?

The error message <none> is not a term isn't helpful for users to understand the problem.

Does this PR introduce any user-facing change?

The exception would be thrown instead of runtime exception from the scala.ScalaReflectionException.

How was this patch tested?

Added a unit test to illustrate the type where expression encoder will fail and trigger the proposed error message.

dongjoon-hyun · 2019-09-09T22:44:06Z

ok to test

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala

SparkQA · 2019-09-10T00:15:13Z

Test build #110374 has finished for PR 25736 at commit ee24c7a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
s\"This could happen if $

SparkQA · 2019-09-10T00:26:13Z

Test build #110376 has finished for PR 25736 at commit c78cd01.

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

HyukjinKwon · 2019-09-10T00:48:37Z

cc @JoshRosen

mickjermsurawong-stripe · 2019-09-10T00:55:02Z

whoops sorry missed the patch here #23062 that caused differing behavior from our local fork. Will make an update

mickjermsurawong-stripe · 2019-09-10T01:13:51Z

This should be good now.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala

HyukjinKwon · 2019-09-10T01:15:41Z

Seems fine to me

mickjermsurawong-stripe · 2019-09-10T01:36:59Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala

+      schemaFor[TraitProductWithoutCompanion]
+    }
+    e.getMessage.contains("Unable to find constructor")
+  }


^ Scrolling up, an existing test is for ScroogeLikeExample where it checks that trait with companion object constructor works.

HeartSaVioR

LGTM

joshrosen-stripe

There's one more case that we might want to test: the case where a trait has a companion but that companion has no constructor / apply method. I believe that the current code will still fall through and error out with the confusing message in that case.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala

SparkQA · 2019-09-10T05:27:35Z

Test build #110383 has finished for PR 25736 at commit 897f737.

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

SparkQA · 2019-09-10T05:45:50Z

Test build #110384 has finished for PR 25736 at commit 0f2c94e.

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

SparkQA · 2019-09-10T07:05:01Z

Test build #110395 has finished for PR 25736 at commit 8ad3769.

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

SparkQA · 2019-09-10T07:05:02Z

Test build #110392 has finished for PR 25736 at commit 47a9da5.

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

SparkQA · 2019-09-10T07:05:02Z

Test build #110396 has finished for PR 25736 at commit 722a090.

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

SparkQA · 2019-09-10T07:05:02Z

Test build #110391 has finished for PR 25736 at commit 88de1e3.

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

HyukjinKwon · 2019-09-10T07:13:02Z

retest this please

HyukjinKwon · 2019-09-10T07:13:52Z

let me leave it to @JoshRosen

JoshRosen

LGTM

SparkQA · 2019-09-10T10:56:10Z

Test build #110405 has finished for PR 25736 at commit 722a090.

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

HyukjinKwon · 2019-09-10T23:43:23Z

Merged to master.

…out companion object constructor ### What changes were proposed in this pull request? - For trait without companion object constructor, currently the method to get constructor parameters `constructParams` in `ScalaReflection` will throw exception. ``` scala.ScalaReflectionException: <none> is not a term at scala.reflect.api.Symbols$SymbolApi.asTerm(Symbols.scala:211) at scala.reflect.api.Symbols$SymbolApi.asTerm$(Symbols.scala:211) at scala.reflect.internal.Symbols$SymbolContextApiImpl.asTerm(Symbols.scala:106) at org.apache.spark.sql.catalyst.ScalaReflection.getCompanionConstructor(ScalaReflection.scala:909) at org.apache.spark.sql.catalyst.ScalaReflection.constructParams(ScalaReflection.scala:914) at org.apache.spark.sql.catalyst.ScalaReflection.constructParams$(ScalaReflection.scala:912) at org.apache.spark.sql.catalyst.ScalaReflection$.constructParams(ScalaReflection.scala:47) at org.apache.spark.sql.catalyst.ScalaReflection.getConstructorParameters(ScalaReflection.scala:890) at org.apache.spark.sql.catalyst.ScalaReflection.getConstructorParameters$(ScalaReflection.scala:886) at org.apache.spark.sql.catalyst.ScalaReflection$.getConstructorParameters(ScalaReflection.scala:47) ``` - Instead this PR would throw exception: ``` Unable to find constructor for type [XXX]. This could happen if [XXX] is an interface or a trait without companion object constructor UnsupportedOperationException: ``` In the normal usage of ExpressionEncoder, this can happen if the type is interface extending `scala.Product`. Also, since this is a protected method, this could have been other arbitrary types without constructor. ### Why are the changes needed? - The error message `<none> is not a term` isn't helpful for users to understand the problem. ### Does this PR introduce any user-facing change? - The exception would be thrown instead of runtime exception from the `scala.ScalaReflectionException`. ### How was this patch tested? - Added a unit test to illustrate the `type` where expression encoder will fail and trigger the proposed error message. Closes apache#25736 from mickjermsurawong-stripe/SPARK-29026. Authored-by: Mick Jermsurawong <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

improve error message

ee24c7a

dongjoon-hyun added the SQL label Sep 9, 2019

dongjoon-hyun reviewed Sep 9, 2019

View reviewed changes

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

improve string interpolation

c78cd01

mickjermsurawong-stripe added 2 commits September 9, 2019 18:06

check at companion object

8c2fce7

improve doc

897f737

HyukjinKwon reviewed Sep 10, 2019

View reviewed changes

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

mickjermsurawong-stripe changed the title ~~[SPARK-29026][SQL] Improve error message when constructor in ScalaReflection isn't found~~ [SPARK-29026][SQL] Improve error message in schemaFor in trait without companion object constructor Sep 10, 2019

improve error message format

0f2c94e

mickjermsurawong-stripe commented Sep 10, 2019

View reviewed changes

HeartSaVioR approved these changes Sep 10, 2019

View reviewed changes

joshrosen-stripe suggested changes Sep 10, 2019

View reviewed changes

mickjermsurawong-stripe added 2 commits September 9, 2019 20:12

handle companion object without apply

88de1e3

variable rename

47a9da5

HyukjinKwon reviewed Sep 10, 2019

View reviewed changes

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

HyukjinKwon reviewed Sep 10, 2019

View reviewed changes

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

dongjoon-hyun reviewed Sep 10, 2019

View reviewed changes

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

mickjermsurawong-stripe added 2 commits September 9, 2019 21:02

nested pattern match

8ad3769

remove extra line

722a090

HyukjinKwon approved these changes Sep 10, 2019

View reviewed changes

JoshRosen approved these changes Sep 10, 2019

View reviewed changes

HyukjinKwon closed this in fa75db2 Sep 10, 2019

[SPARK-29026][SQL] Improve error message in schemaFor in trait without companion object constructor #25736

[SPARK-29026][SQL] Improve error message in schemaFor in trait without companion object constructor #25736

Uh oh!

Conversation

mickjermsurawong-stripe commented Sep 9, 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?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

dongjoon-hyun commented Sep 9, 2019

Uh oh!

Uh oh!

SparkQA commented Sep 10, 2019

Uh oh!

SparkQA commented Sep 10, 2019

Uh oh!

HyukjinKwon commented Sep 10, 2019

Uh oh!

mickjermsurawong-stripe commented Sep 10, 2019

Uh oh!

mickjermsurawong-stripe commented Sep 10, 2019

Uh oh!

Uh oh!

HyukjinKwon commented Sep 10, 2019

Uh oh!

mickjermsurawong-stripe Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

joshrosen-stripe left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Sep 10, 2019

Uh oh!

SparkQA commented Sep 10, 2019

Uh oh!

SparkQA commented Sep 10, 2019

Uh oh!

SparkQA commented Sep 10, 2019

Uh oh!

SparkQA commented Sep 10, 2019

Uh oh!

SparkQA commented Sep 10, 2019

Uh oh!

HyukjinKwon commented Sep 10, 2019

Uh oh!

HyukjinKwon commented Sep 10, 2019

Uh oh!

JoshRosen left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 10, 2019

Uh oh!

HyukjinKwon commented Sep 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[SPARK-29026][SQL] Improve error message in `schemaFor` in trait without companion object constructor #25736

[SPARK-29026][SQL] Improve error message in `schemaFor` in trait without companion object constructor #25736

mickjermsurawong-stripe commented Sep 9, 2019 •

edited

Loading

joshrosen-stripe left a comment •

edited

Loading