Skip to content

Conversation

@Ngone51
Copy link
Member

@Ngone51 Ngone51 commented Jun 24, 2020

What changes were proposed in this pull request?

This PR tries to address the comment: #28645 (comment)
It changes canUpCast/canCast to allow cast from sub UDT to base UDT, in order to achieve the goal to allow UserDefinedType to use ExpressionEncoder to deserialize rows in ScalaUDF as well.

One thing that needs to mention is, even we allow cast from sub UDT to base UDT, it doesn't really do the cast in Cast. Because, yet, sub UDT and base UDT are considered as the same type(because of #16660), see:

override private[sql] def acceptsType(dataType: DataType) = dataType match {
case other: UserDefinedType[_] =>
this.getClass == other.getClass ||
this.userClass.isAssignableFrom(other.userClass)
case _ => false
}

override def equals(other: Any): Boolean = other match {
case that: UserDefinedType[_] => this.acceptsType(that)
case _ => false
}

Therefore, the optimize rule SimplifyCast will eliminate the cast at the end.

Why are the changes needed?

Reduce the special case caused by UserDefinedType in ResolveEncodersInUDF and ScalaUDF.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

It should be covered by the test of SPARK-19311, which is also updated a little in this PR.

@Ngone51
Copy link
Member Author

Ngone51 commented Jun 24, 2020

ping @cloud-fan Please take a look, thanks!

@cloud-fan
Copy link
Contributor

cc @viirya @maropu

@SparkQA
Copy link

SparkQA commented Jun 24, 2020

Test build #124477 has finished for PR 28920 at commit 05e0f9a.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 47fb9d6 Jun 24, 2020
Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

LGTM too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants