You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "findepi (via GitHub)" <gi...@apache.org> on 2024/10/21 11:09:15 UTC
[PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
findepi opened a new pull request, #6608:
URL: https://github.com/apache/arrow-rs/pull/6608
Add counter-part of `Array::null_count`, but counting the logical null values. This will be useful in DataFusion. Current alternative is to compute null mask (via `Array::logical_nulls()`) and do counting on it. Given this might be expensive and verbose, caller may naturally feel steer towards `Array::null_count` which may or may not be applicable, depending on the context.
- follows https://github.com/apache/arrow-rs/pull/4691
# Which issue does this PR close?
- relates to https://github.com/apache/arrow-rs/issues/5208
# Rationale for this change
https://github.com/apache/arrow-rs/pull/4691 changed semantics of `Array::null_count` for eg `NullArray`. DataFusion upgrade to Arrow version with this change introduced a subtle bug, being fixed in https://github.com/apache/datafusion/pull/13029. When working on a fix, it seemed that many usages of `Array::null_count` should be redirect to count logical nulls (not only the one being updated in that PR). Having a function to count logical nulls would be useful, as alternative is computationally more expensive (may involve creation or copying of a null mask).
# What changes are included in this PR?
New `Array::logical_null_count` function.
# Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2427639487
Thank you @alamb @westonpace @tustvold for your time reviewing this!
> I feel fairly strongly that we should not merge this, people will likely use this blindly without appreciating the severe performance penalty it entails.
@tustvold Can you elaborate there the severe performance penalty comes from and what would it take to fix it?
For DataFusion at least the alternative is to all `logical_nulls().map(|n| n.null_count())` which is strictly less performant than the alternative offered here. DataFusion will continue to use this slower path until a faster path exists
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2428554646
> but is this efficient at runtime?
As written in this PR, it will be largely equivalent.
Having slept on it, lets just proceed with this. I don't like it, but then I don't like logical nulls in general, but aside from forking the arrow format we're stuck with them. The types it impacts are relatively niche, and if people care to optimise them, they can
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2426376615
@alamb @tustvold please take a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2427778634
> If this is what you need, which it very often isn't, then you have to call logical_nulls() and get the null count from it. The friction is a feature, not a bug 😅
For my sake, this is fine for me. I had found myself needing the logical null count recently (for array statistics) and using `logical_nulls` wasn't much of a headache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2426552187
> Right, my point is that an accurate logical null count can be very expensive to compute, whereas it is much cheaper to instead determine the existence of any nulls. Whilst this won't serve every use-case, my question is whether DF actually needs accurate null counts all the time
Not all the time, but often enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2428816750
thank you, that makes sense!
and thank you for the PR scrutiny, this is a good thing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2427656780
> For DataFusion at least the alternative is to all logical_nulls().map(|n| n.null_count()) which is strictly less performant than the alternative offered here
The problem is for RunArray, DictionaryArray and UnionArray computing `logical_nulls` is potentially very expensive. Now I accept that there might be a marginal performance win from a specialized logical_null_count implementation, but other than perhaps for NullArray I would expect the difference to largely be a wash. For most types it is a couple of additional atomics, or will be completely dominated by the cost of computing the logical nulls.
The problem with exposing a logical_null_count method is it makes the fact this is effectively computing a fresh null mask implicit, hiding this problem. In fact this PR as written actually regresses `is_nullable` performance, demonstrating this :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2427667809
> The problem is for RunArray, DictionaryArray and UnionArray computing `logical_nulls` is potentially very expensive.
I see your point, thanks for explaining this to me.
Let's turn the question around. What should the caller do, if they want exactly this: know how many (logical) null values are in the array?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#discussion_r1809470943
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -1161,6 +1161,10 @@ impl<T: ArrowPrimitiveType> Array for PrimitiveArray<T> {
self.nulls.as_ref()
}
+ fn logical_null_count(&self) -> usize {
+ self.null_count()
+ }
Review Comment:
To make logical_null_count as performant as null_count for primitive types (where they happen to be equivalent), so that logical_null_count can be used without, or with fewer, performance drawbacks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2428383495
> needing the logical null count recently (for array statistics)
@westonpace good point! this was exactly the case in https://github.com/apache/datafusion/pull/13029 too
but that's not the only place -- DataFusion aggregation accumulators often call `null_count()` and this works **only because** for quite a few array types `null_count()` happens to be "logical null count". But contract-wise, this is a wrong function to call, and doesn't generalize to non-primitive types.
> If this is what you need, which it very often isn't, then you have to call `logical_nulls()` and get the null count from it. The friction is a feature, not a bug 😅
@tustvold I don't mind writing more code (friction), but is this efficient at runtime?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2426474043
@tustvold thank you for taking time to review this PR!
> I'm not sure about this, in all cases where there can be logical nulls, apart from NullArray, this will involve computing the full logical null mask only to throw it away.
Good point. this is what callers that need to find out number of logical nulls have to do today.
Examples: https://github.com/apache/datafusion/blob/e9584bc46ffc574cd65044d4199966402def1d15/datafusion/functions-aggregate/src/count.rs#L605-L607, https://github.com/apache/datafusion/pull/13029
Having this function on the Array itself allows us to provide better implementation.
This PR does this for all primitive types, boolean array and null array
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2426518851
Right, my point is that an accurate logical null count can be very expensive to compute, whereas it is much cheaper to instead determine the existence of any nulls. Whilst this won't serve every use-case, my question is whether DF actually needs accurate null counts all the time, or whether most of the time it is just using them as a proxy for nullability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#discussion_r1809471535
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -316,6 +316,10 @@ impl Array for BooleanArray {
self.nulls.as_ref()
}
+ fn logical_null_count(&self) -> usize {
+ self.null_count()
+ }
Review Comment:
same reasoning as for primitive arrays -- https://github.com/apache/arrow-rs/pull/6608#discussion_r1809470943
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#discussion_r1809461057
##########
arrow-array/src/array/null_array.rs:
##########
@@ -116,8 +117,8 @@ impl Array for NullArray {
(self.len != 0).then(|| NullBuffer::new_null(self.len))
}
- fn is_nullable(&self) -> bool {
- !self.is_empty()
+ fn logical_null_count(&self) -> usize {
+ self.len
Review Comment:
Nevermind, I see that the default impl of `is_nullable` now uses `logical_null_count` :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2427005417
How would this be different than `array.logical_nulls().null_count()` ? Is the idea to pre-compute the null count somehow 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2426460551
I'm not sure about this, in all cases where there can be logical nulls, apart from NullArray, this will involve computing the full logical null mask only to throw it away. This feels like it could be surprising for users, especially when null_count is precomputed.
Perhaps we could discuss making is_nullable precise as opposed to best-effort, as IIUC this is what DF is using this method for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2426637276
cc @joroKr21
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#discussion_r1809454090
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -1161,6 +1161,10 @@ impl<T: ArrowPrimitiveType> Array for PrimitiveArray<T> {
self.nulls.as_ref()
}
+ fn logical_null_count(&self) -> usize {
+ self.null_count()
+ }
Review Comment:
Why overload here?
##########
arrow-array/src/array/null_array.rs:
##########
@@ -116,8 +117,8 @@ impl Array for NullArray {
(self.len != 0).then(|| NullBuffer::new_null(self.len))
}
- fn is_nullable(&self) -> bool {
- !self.is_empty()
+ fn logical_null_count(&self) -> usize {
+ self.len
Review Comment:
This overload I understand (`self.len` is cheaper than allocating `NullBuffer:;new_null`) but what happened to `is_nullable`? Was it just unused?
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -316,6 +316,10 @@ impl Array for BooleanArray {
self.nulls.as_ref()
}
+ fn logical_null_count(&self) -> usize {
+ self.null_count()
+ }
Review Comment:
Why overload here? Is this more efficient somehow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#discussion_r1809462908
##########
arrow-array/src/array/null_array.rs:
##########
@@ -116,8 +117,8 @@ impl Array for NullArray {
(self.len != 0).then(|| NullBuffer::new_null(self.len))
}
- fn is_nullable(&self) -> bool {
- !self.is_empty()
+ fn logical_null_count(&self) -> usize {
+ self.len
Review Comment:
> Nevermind, I see that the default impl of is_nullable now uses logical_null_count :)
Yeah, which is the wrong way round... is_nullable should be the cheaper call...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#discussion_r1809473926
##########
arrow-array/src/array/null_array.rs:
##########
@@ -116,8 +117,8 @@ impl Array for NullArray {
(self.len != 0).then(|| NullBuffer::new_null(self.len))
}
- fn is_nullable(&self) -> bool {
- !self.is_empty()
+ fn logical_null_count(&self) -> usize {
+ self.len
Review Comment:
I can retract changes in `is_nullable` function. I don't care about them.
I changed `is_nullable` because the implementation and documentation seemed to disagree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Array::logical_null_count for inspecting number of null values [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6608:
URL: https://github.com/apache/arrow-rs/pull/6608#issuecomment-2427673920
> know how many (logical) null values are in the array?
If this is what you need, which it very often isn't, then you have to call `logical_nulls()` and get the null count from it. The friction is a feature, not a bug :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org