Uh oh!
There was an error while loading. Please reload this page.
Drop missing from juliaeltype if ValidityBitmap is all valid. - #477
Drop missing from juliaeltype if ValidityBitmap is all valid.#477evetion wants to merge 1 commit into
juliaeltype if ValidityBitmap is all valid.#477Conversation
juliaeltype if ValidityBitmap is all valid.evetion
commented
Jul 27, 2023
Fixes #283 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #477 +/- ##
==========================================
- Coverage 87.50% 5.11% -82.39%
==========================================
Files 26 25 -1 Lines 3280 3208 -72 ==========================================
- Hits 2870 164 -2706 - Misses 410 3044 +2634 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| Base.size(p::ValidityBitmap) = (p.ℓ,) | ||
| nullcount(x::ValidityBitmap) = x.nc | ||
| Base.all(x::ValidityBitmap) = x.nc == 0 |
There was a problem hiding this comment.
I don't love overloading all here; can we just call this allvalid? I don't think we need to overload anything for this.
| f.type.mode, | ||
| typeids, | ||
| Tuple{(juliaeltype(x, buildmetadata(x), convert) for x in f.children)...}, | ||
| Tuple{(juliaeltype(x, buildmetadata(x), convert, false) for x in f.children)...}, |
There was a problem hiding this comment.
Can you help me understand why we hard-code false in a number of places for the allvalid argument? vs. in other places calling all(validity)?
quinnj
left a comment
There was a problem hiding this comment.
I think I'm fine going this direction. I'd like to think a bit on if there may be unintended consequences here, but it seems like it should be strictly an improvement.
evetion
commented
Aug 5, 2023
Ok, I will fix the tests 😃 |
Fixes#384
Fixes#373
This does fail a number of tests, which we can fix after we agree on the direction of this PR.