[GLUTEN-5682][VL] Fix incorrect result when isNull & isNotNull coexist in filter #5670
Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/apache/incubator-gluten/issues Then could you also rename commit message and pull request title in the following format? See also: |
|
Is it only a Spark UT issue? or observed in some SQL? |
It's not just a Spark UT issue. We encounter this bug in our production environment causing result not mismatch. 
|
zjuwangg
changed the title
|
velox related change has been merged via facebookincubator/velox#9718 |
zjuwangg
changed the title
PHILO-HE
changed the title
PHILO-HE
changed the title
| @@ -375,6 +377,8 @@ class SubstraitToVeloxPlanConverter { | |||
|
|
|||
| bool nullAllowed_ = false; | |||
| bool isNull_ = false; | |||
| bool forbidsNullSet_ = false; | |||
| bool isNullSet_ = false; | |||
There was a problem hiding this comment.
Is forbidsNullSet_ logically equivalent to !nullAllowed? And isNullSet_ equivalent to isNull_? If true, we can just use the existing flags (maybe, should also check initialized_ == true?). Thanks!
There was a problem hiding this comment.
Actually not. initialized_ can be set other place not just here two null related setter method.
There was a problem hiding this comment.
Actually not. initialized_ can be set other place not just here two null related setter method.
So we can not according to initialized_ variable to determine whether setNull() or forbidsNull() has been called first.
And now setNull method will set nullAllowed_ to true that could cause unexpected filter behavior.
IMO, forbidsNullSet_ and isNullSet_ make code more clean and readable without other performance cost.
What changes were proposed in this pull request?
Fix the bug founded in issue (Fixes: #5682)
How was this patch tested?
Add ut to cover the change!