You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "findepi (via GitHub)" <gi...@apache.org> on 2024/10/21 10:26:57 UTC
[PR] Fix count on null values [datafusion]
findepi opened a new pull request, #13029:
URL: https://github.com/apache/datafusion/pull/13029
Before the change, the `ValuesExec` containing `NullArray` would
incorrectly report column statistics as being non-null, which would
misinform `AggregateStatistics` optimizer and fold `count(always_null)`
into row count instead of 0.
This commit fixes the column statistics derivation for values with
`NullArray` and therefore fixes execution of logical plans with count
over such values.
Note that the bug was not reproducible using DataFusion SQL frontend,
because in DataFusion SQL the `VALUES (NULL)` doesn't have type
`DataType:Null` (it has some apparently arbitrarily picked type
instead).
As a follow-up, all usages of `Array:null_count` should be inspected.
The function can easily be misused (it returns "physical nulls", which
do not exist for null type).
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix count on null values [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #13029:
URL: https://github.com/apache/datafusion/pull/13029#discussion_r1808569887
##########
datafusion/physical-plan/src/common.rs:
##########
@@ -156,7 +156,11 @@ pub fn compute_record_batch_statistics(
for partition in batches.iter() {
for batch in partition {
for (stat_index, col_index) in projection.iter().enumerate() {
- null_counts[stat_index] += batch.column(*col_index).null_count();
+ null_counts[stat_index] += batch
+ .column(*col_index)
+ .logical_nulls()
+ .map(|nulls| nulls.null_count())
Review Comment:
This could be simplified back if we had something like 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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix count on all null `VALUES` clause [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13029:
URL: https://github.com/apache/datafusion/pull/13029
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix count on null values [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #13029:
URL: https://github.com/apache/datafusion/pull/13029#discussion_r1808507867
##########
datafusion/physical-plan/src/common.rs:
##########
@@ -156,7 +156,11 @@ pub fn compute_record_batch_statistics(
for partition in batches.iter() {
for batch in partition {
for (stat_index, col_index) in projection.iter().enumerate() {
- null_counts[stat_index] += batch.column(*col_index).null_count();
Review Comment:
The meaning of this line was apparently changed in https://github.com/apache/arrow-rs/pull/4691
At least it seems so from this diff line https://github.com/apache/arrow-rs/commit/979a070dc82eeb26b38a8651cac879b2c276c0ed#diff-0cfddb6ef017ce20c5e7f528095956b2433cf2ea2d17e28ba4b515b7c1bd2e57L179
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix count on null values [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13029:
URL: https://github.com/apache/datafusion/pull/13029#issuecomment-2427000620
> Ugh yeah, I don't know why `arrow-rs` made that choice. To me the "physical number of nulls" seems kinda useless. I care about the semantics, not the implementation details 😄
The reason is performance -- I agree it is quite confusing. THe rationale is that arrow-rs I think tries to let the user have maximal control, but that does make the API harder to reason about sometime.
I tried to clarify this in the docs, but I am also still not happy with how easy it is to get confused:
https://docs.rs/arrow/latest/arrow/array/trait.Array.html#tymethod.nulls
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix count on null values [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #13029:
URL: https://github.com/apache/datafusion/pull/13029#issuecomment-2426375511
@alamb @joroKr21 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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix count on all null `VALUES` clause [datafusion]
Posted by "joroKr21 (via GitHub)" <gi...@apache.org>.
joroKr21 commented on PR #13029:
URL: https://github.com/apache/datafusion/pull/13029#issuecomment-2427019848
> I tried to clarify this in the docs, but I am also still not happy with how easy it is to get confused:
https://docs.rs/arrow/latest/arrow/array/trait.Array.html#tymethod.nulls
> The physical representation is efficient, but is sometimes non intuitive for certain array types such as those with nullable child arrays like [DictionaryArray::values](https://docs.rs/arrow/latest/arrow/array/struct.DictionaryArray.html#method.values), [RunArray::values](https://docs.rs/arrow/latest/arrow/array/struct.RunArray.html#method.values) or [UnionArray](https://docs.rs/arrow/latest/arrow/array/struct.UnionArray.html), or without a null buffer, such as [NullArray](https://docs.rs/arrow/latest/arrow/array/struct.NullArray.html).
Yeah, I haven't read the Arrow spec so I'm not sure why you would do that (encode nulls in the child arrays)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org