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/11/12 07:46:44 UTC
[PR] Improve default implementation of Array::is_nullable [arrow-rs]
findepi opened a new pull request, #6721:
URL: https://github.com/apache/arrow-rs/pull/6721
# Description
Improve default implementation of Array::is_nullable
Since is_nullable returns `false` if the array is guaranteed to not contain any logical nulls, the correct default implementation could leverage `self.logical_null_count` more appropriately than `self.null_count`.
To reduce chance of negative surprises in downstream projects, we could introduce the new behavior under `is_logically_nullable` name and deprecate `is_nullable` without changing it.
# Which issue does this PR close?
None
# Rationale for this change
Fix technically incorrect default implementation of `Array::is_nullable`
# What changes are included in this PR?
Change default implementation of `Array::is_nullable`.
# 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] Improve default implementation of Array::is_nullable [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6721:
URL: https://github.com/apache/arrow-rs/pull/6721#issuecomment-2485066220
thank you @tustvold for merging https://github.com/apache/arrow-rs/pull/6740.
i rebased this one. Looking forward to further advice.
--
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] Improve default implementation of Array::is_nullable [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #6721:
URL: https://github.com/apache/arrow-rs/pull/6721#discussion_r1845248260
##########
arrow-array/src/array/mod.rs:
##########
@@ -315,8 +315,7 @@ pub trait Array: std::fmt::Debug + Send + Sync {
/// even if the nulls present in [`DictionaryArray::values`] are not referenced by any key,
/// and therefore would not appear in [`Array::logical_nulls`].
fn is_nullable(&self) -> bool {
- // TODO this is not necessarily perfect default implementation, since null_count() and logical_null_count() are not always equivalent
- self.null_count() != 0
+ self.logical_null_count() != 0
Review Comment:
i added explicit implementation of `DictionaryArray::logical_null_count`
i am not sure what to do about UnionArray. `UnionArray::logical_nulls` has non trivial logic. Can you please advise how this should be approached the best way? should I just copy that function?
cc @alamb
--
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] Improve default implementation of Array::is_nullable [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #6721:
URL: https://github.com/apache/arrow-rs/pull/6721
--
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] Improve default implementation of Array::is_nullable [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6721:
URL: https://github.com/apache/arrow-rs/pull/6721#discussion_r1837757269
##########
arrow-array/src/array/mod.rs:
##########
@@ -315,8 +315,7 @@ pub trait Array: std::fmt::Debug + Send + Sync {
/// even if the nulls present in [`DictionaryArray::values`] are not referenced by any key,
/// and therefore would not appear in [`Array::logical_nulls`].
fn is_nullable(&self) -> bool {
- // TODO this is not necessarily perfect default implementation, since null_count() and logical_null_count() are not always equivalent
- self.null_count() != 0
+ self.logical_null_count() != 0
Review Comment:
Can we provide alternative implementations DictionaryArray and UnionArray as part of this, currently this will significantly regress performance for them
--
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] Improve default implementation of Array::is_nullable [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #6721:
URL: https://github.com/apache/arrow-rs/pull/6721#discussion_r1845246732
##########
arrow-array/src/array/mod.rs:
##########
@@ -315,8 +315,7 @@ pub trait Array: std::fmt::Debug + Send + Sync {
/// even if the nulls present in [`DictionaryArray::values`] are not referenced by any key,
/// and therefore would not appear in [`Array::logical_nulls`].
fn is_nullable(&self) -> bool {
- // TODO this is not necessarily perfect default implementation, since null_count() and logical_null_count() are not always equivalent
- self.null_count() != 0
+ self.logical_null_count() != 0
Review Comment:
the `logical_null_count` implementation should obviously be consistent with `logical_nulls` implementation, so let's do https://github.com/apache/arrow-rs/pull/6740 first
--
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] Improve default implementation of Array::is_nullable [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6721:
URL: https://github.com/apache/arrow-rs/pull/6721#issuecomment-2470028243
We should also update the doc comment for this as well, as currently it is highlighted as pessimisable
--
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