Minor: Clean up the code of MutableArrayData #1763
Conversation
| if arrays.iter().any(|array| array.null_count() > 0) { | ||
| use_nulls = true; | ||
| }; | ||
| let use_nulls = use_nulls | arrays.iter().any(|array| array.null_count() > 0); |
There was a problem hiding this comment.
Short circuit here.
There was a problem hiding this comment.
I think this is incorrect, as described above use_nulls is a hint, and should be false if the only source of nulls are the arras themselves.
To explain why, you might have a number of source arrays that don't contain any nulls, but then call extend_nulls.
If it helps I tried to clean this up a bit in #1225 but abandoned it for lack of time - if you wanted to pick it up again...
There was a problem hiding this comment.
I think this is incorrect, as described above use_nulls is a hint, and should be false if the only source of nulls are the arras themselves.
Thank you @tustvold for your review. I am still a little confused about it.
This is the truth table in my mind:
the input value of use_nulls |
arrays.any(null_count > 0) |
final value of use_nulls |
|---|---|---|
| true | true | true |
| true | false | true |
| false | true | true |
| false | false | false |
Although I not very clear about the whole logic of the function with_capacities, for use_nulls I think this is the logic.
There was a problem hiding this comment.
Aah yes, was a long day and brain was fried 😅
There was a problem hiding this comment.
Aah yes, was a long day and brain was fried 😅
Haha, summer is coming!
Codecov Report
@@ Coverage Diff @@
## master #1763 +/- ##
=======================================
Coverage 83.48% 83.48%
=======================================
Files 196 196
Lines 55923 55907 -16
=======================================
- Hits 46686 46676 -10
+ Misses 9237 9231 -6
Continue to review full report at Codecov.
|
Signed-off-by: remzi 13716567376yh@gmail.com
Which issue does this PR close?
None.
Rationale for this change
Make the code cleaner.
What changes are included in this PR?
use_null...
Are there any user-facing changes?
No.