[SPARK-25044][SQL][followup] add back UserDefinedFunction.inputTypes #22319

cloud-fan · 2018-09-03T05:03:48Z

What changes were proposed in this pull request?

This is a followup of #22259 .

Scala case class has a wide surface: apply, unapply, accessors, copy, etc.

In #22259 , we change the type of UserDefinedFunction.inputTypes from Option[Seq[DataType]] to Option[Seq[Schema]]. This breaks backward compatibility.

This PR changes the type back, and use a var to keep the new nullable info.

How was this patch tested?

N/A

cloud-fan · 2018-09-03T05:05:07Z

cc @srowen @gatorsmile

SparkQA · 2018-09-03T05:14:08Z

Test build #95598 has finished for PR 22319 at commit e9c6fbc.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-09-03T05:20:43Z

sql/core/src/main/scala/org/apache/spark/sql/expressions/UserDefinedFunction.scala


  private var _nameOption: Option[String] = None
  private var _nullable: Boolean = true
  private var _deterministic: Boolean = true

+  // This is to keep backward compatibility for this case class.
+  // TODO: revisit this case class in Spark 3.0, and narrow down the public surface.
+  def inputTypes: Option[Seq[DataType]] = {


@cloud-fan, I think this still breaks compatibility when UserDefinedFunction's used in a pattern match.

HyukjinKwon · 2018-09-03T05:49:48Z

sql/core/src/main/scala/org/apache/spark/sql/expressions/UserDefinedFunction.scala

+object UserDefinedFunction {
+  // This is to keep backward compatibility for this case class.
+  // TODO: revisit this case class in Spark 3.0, and narrow down the public surface.
+  def unapply(arg: UserDefinedFunction): Option[(AnyRef, DataType, Option[Seq[DataType]])] = {


Doesn't this still break binary compatibility since we bind to another signature?

I checked locally and apparently not - looks explicit unapply here and unapply from case class are handled as the same signature from my cursory test.

HyukjinKwon · 2018-09-03T05:53:59Z

sql/core/src/main/scala/org/apache/spark/sql/expressions/UserDefinedFunction.scala

@@ -129,3 +135,11 @@ case class UserDefinedFunction protected[sql] (
    }
  }
 }
+
+object UserDefinedFunction {


private[sql] since we don't explicitly mention expressions package is meant to be internal.

I am okay if this is the only way.

Here I'm adding back the public unapply method, so this must be public.

SparkQA · 2018-09-03T05:54:31Z

Test build #95599 has finished for PR 22319 at commit 5acb1df.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-09-03T05:59:50Z

Test build #95600 has finished for PR 22319 at commit 2245395.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-09-03T11:35:31Z

sql/core/src/main/scala/org/apache/spark/sql/expressions/UserDefinedFunction.scala

+object UserDefinedFunction {
+  // This is to keep backward compatibility for this case class.
+  // TODO: revisit this case class in Spark 3.0, and narrow down the public surface.
+  def unapply(obj: Any): Option[(AnyRef, DataType, Option[Seq[DataType]])] = obj match {


Wenchen, can we have a small test to check if the previous pattern matching works or not, and can you double check this by mimicking the case by javap -v manually at least since this looks not being checked?

SparkQA · 2018-09-03T11:53:42Z

Test build #95611 has finished for PR 22319 at commit 01a51ca.

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

cloud-fan · 2018-09-03T13:39:52Z

It's too hard to work around the case class compatibility issue. I'm leaving it unchanged, and add private mutable variables to store the nullable info.

SparkQA · 2018-09-03T13:46:45Z

Test build #95626 has finished for PR 22319 at commit c77db87.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-09-03T13:59:29Z

I prefer the current way as well.

SparkQA · 2018-09-03T18:05:01Z

Test build #95630 has finished for PR 22319 at commit d290fec.

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

gatorsmile · 2018-09-04T04:29:52Z

sql/core/src/main/scala/org/apache/spark/sql/expressions/UserDefinedFunction.scala

@@ -41,12 +41,16 @@ import org.apache.spark.sql.types.DataType
 case class UserDefinedFunction protected[sql] (
    f: AnyRef,
    dataType: DataType,
-    inputTypes: Option[Seq[ScalaReflection.Schema]]) {
+    inputTypes: Option[Seq[DataType]]) {


+1. This is why we added _nameOption, _nullable and _deterministic in 2.3 release.

Please also remove the changes of MimaExcludes.scala made in #22259

SparkQA · 2018-09-04T04:45:37Z

Test build #95644 has finished for PR 22319 at commit 4791240.

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

SparkQA · 2018-09-04T07:39:14Z

Test build #95660 has finished for PR 22319 at commit 2b2c894.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2018-09-04T16:53:25Z

Hm, still need one MiMa exclusion:

[error]  * the type hierarchy of object org.apache.spark.sql.expressions.UserDefinedFunction is different in current version. Missing types {scala.runtime.AbstractFunction3}
[error]    filter with: ProblemFilters.exclude[MissingTypesProblem]("org.apache.spark.sql.expressions.UserDefinedFunction$")

HyukjinKwon · 2018-09-05T05:16:10Z

Shall we update migration guide about the compatibility?

cloud-fan · 2018-09-05T05:33:00Z

I've fixed all the compatibility issues. Is there something else we want to let users know?

SparkQA · 2018-09-05T05:46:47Z

Test build #95691 has finished for PR 22319 at commit 9e060a4.

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

cloud-fan · 2018-09-05T05:49:14Z

retest this please

HyukjinKwon · 2018-09-05T05:54:18Z

sql/core/src/main/scala/org/apache/spark/sql/expressions/UserDefinedFunction.scala

+
+// We have to use a name different than `UserDefinedFunction` here, to avoid breaking the binary
+// compatibility of the auto-generate UserDefinedFunction object.
+private[sql] object SparkUserDefinedFunction {


Oh, so we're working around by new object here. SGTM. I assumed mutable status and copy stuff but trivial or not an issue.

gatorsmile · 2018-09-05T06:45:54Z

LGTM

SparkQA · 2018-09-05T07:05:01Z

Test build #95696 has finished for PR 22319 at commit 9e060a4.

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

cloud-fan · 2018-09-05T07:06:15Z

retest this please

SparkQA · 2018-09-05T11:40:30Z

Test build #95700 has finished for PR 22319 at commit 9e060a4.

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

srowen

Yeah it is a little hacky internally but maximizes compatibility.

cloud-fan · 2018-09-05T13:13:33Z

thanks, merging to master!

add back UserDefinedFunction.inputTypes

e9c6fbc

srowen approved these changes Sep 3, 2018

View reviewed changes

HyukjinKwon reviewed Sep 3, 2018

View reviewed changes

add back unapply

5acb1df

HyukjinKwon reviewed Sep 3, 2018

View reviewed changes

fix

2245395

HyukjinKwon reviewed Sep 3, 2018

View reviewed changes

fix

01a51ca

HyukjinKwon reviewed Sep 3, 2018

View reviewed changes

another fix

c77db87

srowen approved these changes Sep 3, 2018

View reviewed changes

fix apply conflict

d290fec

fix a mistake

4791240

gatorsmile reviewed Sep 4, 2018

View reviewed changes

revert mima

2b2c894

HyukjinKwon approved these changes Sep 4, 2018

View reviewed changes

another fix

9e060a4

HyukjinKwon reviewed Sep 5, 2018

View reviewed changes

srowen approved these changes Sep 5, 2018

View reviewed changes

asfgit closed this in 341b55a Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-25044][SQL][followup] add back UserDefinedFunction.inputTypes #22319

[SPARK-25044][SQL][followup] add back UserDefinedFunction.inputTypes #22319

cloud-fan commented Sep 3, 2018 •

edited

Loading

cloud-fan commented Sep 3, 2018

SparkQA commented Sep 3, 2018

HyukjinKwon Sep 3, 2018 •

edited

Loading

HyukjinKwon Sep 3, 2018

HyukjinKwon Sep 3, 2018

HyukjinKwon Sep 3, 2018

HyukjinKwon Sep 3, 2018

cloud-fan Sep 3, 2018

SparkQA commented Sep 3, 2018

SparkQA commented Sep 3, 2018

HyukjinKwon Sep 3, 2018 •

edited

Loading

SparkQA commented Sep 3, 2018

cloud-fan commented Sep 3, 2018

SparkQA commented Sep 3, 2018

HyukjinKwon commented Sep 3, 2018

SparkQA commented Sep 3, 2018

gatorsmile Sep 4, 2018

SparkQA commented Sep 4, 2018

SparkQA commented Sep 4, 2018

srowen commented Sep 4, 2018

HyukjinKwon commented Sep 5, 2018

cloud-fan commented Sep 5, 2018

SparkQA commented Sep 5, 2018

cloud-fan commented Sep 5, 2018

HyukjinKwon Sep 5, 2018

gatorsmile commented Sep 5, 2018

SparkQA commented Sep 5, 2018

cloud-fan commented Sep 5, 2018

SparkQA commented Sep 5, 2018

srowen left a comment

cloud-fan commented Sep 5, 2018

[SPARK-25044][SQL][followup] add back UserDefinedFunction.inputTypes #22319

[SPARK-25044][SQL][followup] add back UserDefinedFunction.inputTypes #22319

Conversation

cloud-fan commented Sep 3, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

cloud-fan commented Sep 3, 2018

SparkQA commented Sep 3, 2018

HyukjinKwon Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

HyukjinKwon Sep 3, 2018

Choose a reason for hiding this comment

HyukjinKwon Sep 3, 2018

Choose a reason for hiding this comment

HyukjinKwon Sep 3, 2018

Choose a reason for hiding this comment

HyukjinKwon Sep 3, 2018

Choose a reason for hiding this comment

cloud-fan Sep 3, 2018

Choose a reason for hiding this comment

SparkQA commented Sep 3, 2018

SparkQA commented Sep 3, 2018

HyukjinKwon Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Sep 3, 2018

cloud-fan commented Sep 3, 2018

SparkQA commented Sep 3, 2018

HyukjinKwon commented Sep 3, 2018

SparkQA commented Sep 3, 2018

gatorsmile Sep 4, 2018

Choose a reason for hiding this comment

SparkQA commented Sep 4, 2018

SparkQA commented Sep 4, 2018

srowen commented Sep 4, 2018

HyukjinKwon commented Sep 5, 2018

cloud-fan commented Sep 5, 2018

SparkQA commented Sep 5, 2018

cloud-fan commented Sep 5, 2018

HyukjinKwon Sep 5, 2018

Choose a reason for hiding this comment

gatorsmile commented Sep 5, 2018

SparkQA commented Sep 5, 2018

cloud-fan commented Sep 5, 2018

SparkQA commented Sep 5, 2018

srowen left a comment

Choose a reason for hiding this comment

cloud-fan commented Sep 5, 2018

cloud-fan commented Sep 3, 2018 •

edited

Loading

HyukjinKwon Sep 3, 2018 •

edited

Loading

HyukjinKwon Sep 3, 2018 •

edited

Loading