Uh oh!
There was an error while loading. Please reload this page.
[SPARK-16582][SQL] Explicitly define isNull = false for non-nullable expressions - #14227
[SPARK-16582][SQL] Explicitly define isNull = false for non-nullable expressions#14227sameeragarwal wants to merge 1 commit into
Conversation
SparkQA
commented
Jul 16, 2016
Test build #62396 has finished for PR 14227 at commit
|
sameeragarwal
commented
Jul 16, 2016
cc @ericl |
ericl
commented
Jul 16, 2016
Lgtm |
rxin
commented
Jul 16, 2016
Merging in master/2.0. |
…expressions ## What changes were proposed in this pull request? This patch is just a slightly safer way to fix the issue we encountered in #14168 should this pattern re-occur at other places in the code. ## How was this patch tested? Existing tests. Also, I manually tested that it fixes the problem in SPARK-16514 without having the proposed change in #14168 Author: Sameer Agarwal <sameerag@cs.berkeley.edu> Closes#14227 from sameeragarwal/codegen. (cherry picked from commit a1ffbad) Signed-off-by: Reynold Xin <rxin@databricks.com>
| """) | ||
| } else { | ||
| ev.copy(code = s""" | ||
| boolean ${ev.isNull} = false; |
There was a problem hiding this comment.
I don't quite understand this, we explicitly define isNull = "false" below, how could ev.isNull be referenced later?
There was a problem hiding this comment.
@cloud-fan ideally it shouldn't be referenced but we recently discovered these incorrect patterns in the code (e.g., https://github.com/apache/spark/pull/14168/files#diff-39298b470865a4cbc67398a4ea11e767L290) where the non-nullable branches were not explicitly tested. This change was just a defensive measure for codegen to not break in those cases. Let me also add a comment here to reduce confusion.
What changes were proposed in this pull request?
This patch is just a slightly safer way to fix the issue we encountered in #14168 should this pattern re-occur at other places in the code.
How was this patch tested?
Existing tests. Also, I manually tested that it fixes the problem in SPARK-16514 without having the proposed change in #14168