[SPARK-24913][SQL] Make AssertNotNull and AssertTrue non-deterministic #22947
Conversation
|
Test build #98472 has finished for PR 22947 at commit
|
|
|
||
| override def nullable: Boolean = true | ||
|
|
||
| override lazy val deterministic: Boolean = false |
There was a problem hiding this comment.
hmm, this two expressions are very commonly used in other expressions' children. Making them non-deterministic will make such expressions as non-deterministic too, e.g., encoders. It might have big impact.
There was a problem hiding this comment.
yes, that's true. The change may be big indeed. I am open to other proposals, as creating a new sideEffect flag in the Expression.
There was a problem hiding this comment.
Because of this, I'm leaning towards creating a new flag instead of making them non-deterministic.
There was a problem hiding this comment.
I agree, thanks you all the comments. I'll try and add a new flag. I'll update the PR as soon as I have a solution for that. Probably it will take some time, though, as I will have to recheck all the usage of determistic.... Thanks.
There was a problem hiding this comment.
https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAssertTrue.java#L45 Hive also marks it as non-deterministic.
cc @maryannxue too
There was a problem hiding this comment.
@gatorsmile thanks for your comment. I think the main issue with making this non-determistic is that we have basically to skip the sanity check at
AssertNotNull is a lot of other different locations. Another option may be to special case AssertNotNull and avoid that check for it... What do you think?
There was a problem hiding this comment.
any comment on this @gatorsmile @maryannxue ?
|
Test build #98476 has finished for PR 22947 at commit
|
|
Test build #98477 has finished for PR 22947 at commit
|
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for pinging me, @mgaido91 .
As I'm the one who raised the issue about that optimizer and AssertX, I'm not sure about making this as non-deterministic. The reason is simply it's not logically correct.
I'll review this back after Jenkins passes.
|
Test build #98560 has finished for PR 22947 at commit
|
|
any more comments on this? |
|
We're closing this PR because it hasn't been updated in a while. If you'd like to revive this PR, please reopen it! |
6 participants
What changes were proposed in this pull request?
#21848 introduces an optimization which causes wrong behavior with
AssertNotNullandAssertTrue. This PR makes the two mentioned expressions as non-deterministic in order to avoid skipping evaluating them.How was this patch tested?
added UT