-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-29100][SQL] Fix compilation error in codegen with switch from InSet expression #25806
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
|
Thank you for fixing this, @viirya . |
|
Test build #110660 has finished for PR 25806 at commit
|
| break; | ||
| """) | ||
|
|
||
| val switchCode = if (caseBranches.size > 0) { |
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.
We don't need to compute caseBranches, too, if hset is empty?
val valueGen = child.genCode(ctx)
val switchCode = if (hset.isEmpty) {
val caseValuesGen = hset.filter(_ != null).map(Literal(_).genCode(ctx))
val caseBranches = caseValuesGen.map(literal =>
code"""
case ${literal.value}:
${ev.value} = true;
break;
""")
code"""
switch (${valueGen.value}) {
${caseBranches.mkString("\n")}
default:
${ev.isNull} = $hasNull;
}
"""
} else {
s"${ev.isNull} = $hasNull;"
}
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.
btw, can we fold this expr in the optimizer when hset is empty?
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.
if hset is empty, the computation of caseBranches is noop, right?
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.
This is a special case. Usually InSet wouldn't have empty set, as we have a threshold to convert In to Inset.
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.
Ur, I see.
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.
LGTM except for one minor comment.
|
thanks, merging to master! |
|
thanks @maropu @cloud-fan @dongjoon-hyun |
What changes were proposed in this pull request?
When InSet generates Java switch-based code, if the input set is empty, we don't generate switch condition, but a simple expression that is default case of original switch.
Why are the changes needed?
SPARK-26205 adds an optimization to InSet that generates Java switch condition for certain cases. When the given set is empty, it is possibly that codegen causes compilation error:
Does this PR introduce any user-facing change?
Yes. Previously, when users have InSet against an empty set, generated code causes compilation error. This patch fixed it.
How was this patch tested?
Unit test added.