-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-24165][SQL][branch-2.3] Fixing conditional expressions to handle nullability of nested types #21747
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
Conversation
…le nullability of nested types
|
Test build #92856 has finished for PR 21747 at commit
|
|
retest this please |
| * data types of all child expressions. The collection must not be empty. | ||
| */ | ||
| @transient | ||
| lazy val inputTypesForMerging: Seq[DataType] = children.map(_.dataType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: @transient lazy val
| StructType(newFields) | ||
| } | ||
|
|
||
| override def dataType: DataType = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we change def into lazy val? I feel a little weird that the two requirement checks are invoked every dataType called.
| extends ComplexTypeMergingExpression { | ||
|
|
||
| @transient | ||
| override lazy val inputTypesForMerging: Seq[DataType] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: @transient lazy val
|
Test build #92859 has finished for PR 21747 at commit
|
|
retest this please |
|
Test build #92862 has finished for PR 21747 at commit
|
|
retest this please |
1 similar comment
|
retest this please |
|
Test build #92912 has finished for PR 21747 at commit
|
|
retest this please |
|
Thanks guys for triggering the builds, but I must implement a fix first. It's failing since the code is accessing |
|
Test build #92914 has finished for PR 21747 at commit
|
|
Test build #92925 has finished for PR 21747 at commit
|
|
ah, that's hard to fix, maybe just leave it, since Spark 2.4 is coming. |
|
|
|
IMHO, we could directly use If you don't like this approach for any reason, I'm happy to close the PR. :-) |
|
Test build #92933 has finished for PR 21747 at commit
|
|
Can one of the admins verify this patch? |
|
Hi, @mn-mikke and @cloud-fan and @maropu .
|
|
If nobody has any objections, I'm happy to close this PR. |
|
To backport this fix, we need to backport another improvement PR that allows accessing SQLConf at executor side, which violates the backport rule. I think it's ok to have this fix in 2.4 only. |
|
Thank you for the decision, @mn-mikke and @cloud-fan ! |
What changes were proposed in this pull request?
This PR is proposing a fix for the output data type of
IfandCaseWhenexpression. Upon till now, the implementation of exprassions has ignored nullability of nested types from different execution branches and returned the type of the first branch.This could lead to an unwanted
NullPointerExceptionfrom other expressions depending on aIf/CaseWhenexpression.Example:
Exception:
Output schema:
How was this patch tested?
New test cases added into