Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-11011][SQL] Narrow type of UDT serialization #11379

Closed
wants to merge 2 commits into from
Closed

[SPARK-11011][SQL] Narrow type of UDT serialization #11379

wants to merge 2 commits into from

Conversation

jodersky
Copy link
Member

What changes were proposed in this pull request?

Narrow down the parameter type of UserDefinedType#serialize(). Currently, the parameter type is Any, however it would logically make more sense to narrow it down to the type of the actual user defined type.

How was this patch tested?

Existing tests were successfully run on local machine.

@@ -50,11 +50,8 @@ abstract class UserDefinedType[UserType] extends DataType with Serializable {

/**
* Convert the user type to a SQL datum
*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also be a good idea to update the documentation. In this case, what exactly is a SQL datum?

@SparkQA
Copy link

SparkQA commented Feb 26, 2016

Test build #52006 has finished for PR 11379 at commit 8f49d91.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class UserDefinedType[UserType >: Null] extends DataType with Serializable

@SparkQA
Copy link

SparkQA commented Feb 26, 2016

Test build #52010 has finished for PR 11379 at commit 8164823.

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

@jodersky
Copy link
Member Author

jodersky commented Mar 7, 2016

Latest commit only fixes a conflict in MiMa exclusion rules. Retesting should not be needed.

@SparkQA
Copy link

SparkQA commented Mar 7, 2016

Test build #52579 has finished for PR 11379 at commit 61779ea.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jodersky
Copy link
Member Author

jodersky commented Mar 7, 2016

Whoops! Thanks for reminding me about the usefulness of testing, jenkins :)

@frreiss
Copy link
Contributor

frreiss commented Mar 7, 2016

LGTM. Strange how cleaner Scala can fail the Scala style tests ;)

@jodersky
Copy link
Member Author

jodersky commented Mar 7, 2016

It was actually a missing comma in an sbt file (MimaExcludes.scala in project/). Arguably a syntax error can be considered bad style ;)

@SparkQA
Copy link

SparkQA commented Mar 7, 2016

Test build #52584 has finished for PR 11379 at commit e5fb44e.

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

@jodersky
Copy link
Member Author

jodersky commented Mar 7, 2016

Seems unrelated and works locally. Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Mar 8, 2016

Test build #52600 has finished for PR 11379 at commit e5fb44e.

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

@mengxr
Copy link
Contributor

mengxr commented Mar 16, 2016

+1. There was a bug in SQL internal type conversions, which might call serialize twice. This is why Any was used. The bug was fixed but I forgot to change the type back.

@yhuai Could you make a final pass?

@yhuai
Copy link
Contributor

yhuai commented Mar 16, 2016

LGTM

@mengxr
Copy link
Contributor

mengxr commented Mar 17, 2016

Merged into master. Thanks!

@asfgit asfgit closed this in d4d8493 Mar 17, 2016
roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016
## What changes were proposed in this pull request?

Narrow down the parameter type of `UserDefinedType#serialize()`. Currently, the parameter type is `Any`, however it would logically make more sense to narrow it down to the type of the actual user defined type.

## How was this patch tested?

Existing tests were successfully run on local machine.

Author: Jakob Odersky <jakob@odersky.com>

Closes apache#11379 from jodersky/SPARK-11011-udt-types.
@jodersky jodersky deleted the SPARK-11011-udt-types branch March 31, 2016 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants