You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "efredine (via GitHub)" <gi...@apache.org> on 2024/07/05 20:58:12 UTC
[PR] Fix data page statistics when all rows are null in a data page [datafusion]
efredine opened a new pull request, #11295:
URL: https://github.com/apache/datafusion/pull/11295
## Which issue does this PR close?
Closes #11280.
## Rationale for this change
When all rows for a data page are null the min and max statistics should be present but null. Some of the data page statistics iterators were incorrectly omitting statistics rather than setting them to null. This results in an array whose length is different from the number of data pages.
## What changes are included in this PR?
Adds test for data page statistics for all data types when all rows in a data page are null. Fixes data page statistics iterators that fail these tests.
## Are these changes tested?
Yes.
## 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@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 data page statistics when all rows are null in a data page [datafusion]
Posted by "efredine (via GitHub)" <gi...@apache.org>.
efredine commented on code in PR #11295:
URL: https://github.com/apache/datafusion/pull/11295#discussion_r1667208743
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -600,6 +601,31 @@ make_data_page_stats_iterator!(
Index::DOUBLE,
f64
);
Review Comment:
Just consolidating these together.
--
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 data page statistics when all rows are null in a data page [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on code in PR #11295:
URL: https://github.com/apache/datafusion/pull/11295#discussion_r1667735581
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -823,11 +819,11 @@ macro_rules! get_data_page_statistics {
Float16Array::from_iter(
[<$stat_type_prefix Float16DataPageStatsIterator>]::new($iterator)
.map(|x| {
- x.into_iter().filter_map(|x| {
- x.and_then(|x| Some(from_bytes_to_f16(x.data())))
+ x.into_iter().map(|x| {
+ x.and_then(|x| from_bytes_to_f16(x.data()))
})
})
- .flatten()
+ .flatten().collect::<Vec<_>>()
Review Comment:
`collect` may be unnecessary here?
--
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 data page statistics when all rows are null in a data page [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11295:
URL: https://github.com/apache/datafusion/pull/11295#discussion_r1667745320
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -919,16 +915,28 @@ macro_rules! get_data_page_statistics {
})
},
Some(DataType::FixedSizeBinary(size)) => {
- Ok(Arc::new(
- FixedSizeBinaryArray::try_from_iter(
- [<$stat_type_prefix FixedLenByteArrayDataPageStatsIterator>]::new($iterator)
- .flat_map(|x| x.into_iter())
- .filter_map(|x| x)
- ).unwrap_or_else(|e| {
- log::debug!("FixedSizeBinary statistics is invalid: {}", e);
- FixedSizeBinaryArray::new(*size, vec![].into(), None)
- })
- ))
+ let mut builder = FixedSizeBinaryBuilder::new(*size);
Review Comment:
this looks like a nice change to me
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -823,11 +819,11 @@ macro_rules! get_data_page_statistics {
Float16Array::from_iter(
[<$stat_type_prefix Float16DataPageStatsIterator>]::new($iterator)
.map(|x| {
- x.into_iter().filter_map(|x| {
- x.and_then(|x| Some(from_bytes_to_f16(x.data())))
+ x.into_iter().map(|x| {
+ x.and_then(|x| from_bytes_to_f16(x.data()))
})
})
- .flatten()
+ .flatten().collect::<Vec<_>>()
Review Comment:
That is an excellent call. It looks to me like this change adds `collect` to make this use consistent with the other arms. I tried removing `collect` locally and indeed the tests still pass
I think `collect` is necessary in some cases to avoid a panic due to the iterator was not sized
like when I removed it from the BooleanArray I got an error like this
```
Iterator must be sized
thread 'parquet::arrow_statistics::test_data_page_stats_with_all_null_page' panicked at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/arrow-array-52.1.0/src/array/boolean_array.rs:407:33:
Iterator must be sized
stack backtrace:
```
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -503,6 +512,71 @@ async fn test_multiple_data_pages_nulls_and_negatives() {
.run()
}
+#[tokio::test]
+async fn test_data_page_stats_with_all_null_page() {
+ for data_type in &[
+ DataType::Boolean,
+ DataType::UInt64,
+ DataType::UInt32,
+ DataType::UInt16,
+ DataType::UInt8,
+ DataType::Int64,
+ DataType::Int32,
+ DataType::Int16,
+ DataType::Int8,
+ DataType::Float16,
+ DataType::Float32,
+ DataType::Float64,
+ DataType::Date32,
+ DataType::Date64,
+ DataType::Time32(TimeUnit::Millisecond),
+ DataType::Time32(TimeUnit::Second),
+ DataType::Time64(TimeUnit::Microsecond),
+ DataType::Time64(TimeUnit::Nanosecond),
+ DataType::Timestamp(TimeUnit::Second, None),
+ DataType::Timestamp(TimeUnit::Millisecond, None),
+ DataType::Timestamp(TimeUnit::Microsecond, None),
+ DataType::Timestamp(TimeUnit::Nanosecond, None),
+ DataType::Binary,
+ DataType::LargeBinary,
+ DataType::FixedSizeBinary(3),
+ DataType::Utf8,
+ DataType::LargeUtf8,
+ DataType::Dictionary(Box::new(DataType::Int32), Box::new(DataType::Utf8)),
+ DataType::Decimal128(8, 2), // as INT32
+ DataType::Decimal128(10, 2), // as INT64
+ DataType::Decimal128(20, 2), // as FIXED_LEN_BYTE_ARRAY
+ DataType::Decimal256(8, 2), // as INT32
+ DataType::Decimal256(10, 2), // as INT64
+ DataType::Decimal256(20, 2), // as FIXED_LEN_BYTE_ARRAY
+ ] {
+ let batch =
+ RecordBatch::try_from_iter(vec![("col", new_null_array(data_type, 4))])
+ .expect("record batch creation");
+
+ let reader =
+ build_parquet_file(4, Some(EnabledStatistics::Page), Some(4), vec![batch]);
+
+ let expected_data_type = match data_type {
+ DataType::Dictionary(_, value_type) => value_type.as_ref(),
+ _ => data_type,
+ };
+
+ // There is one data page with 4 nulls
+ // The statistics should be present but null
+ Test {
Review Comment:
I verified that this test covered the code changes by running the test without the code changes and it failed as expected.
```text
thread 'parquet::arrow_statistics::test_data_page_stats_with_all_null_page' panicked at datafusion/core/tests/parquet/arrow_statistics.rs:276:13:
assertion `left == right` failed: col: Mismatch with expected data page minimums
left: PrimitiveArray<UInt64>
[
]
right: PrimitiveArray<UInt64>
[
null,
]
stack backtrace:
0: rust_begin_unwind
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:652:5
1: core::panicking::panic_fmt
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/panicking.rs:72:14
2: core::panicking::assert_failed_inner
3: core::panicking::assert_failed
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/panicking.rs:364:5
4: parquet_exec::parquet::arrow_statistics::Test::run_checks
at ./tests/parquet/arrow_statistics.rs:276:13
5: parquet_exec::parquet::arrow_statistics::Test::run
at ./tests/parquet/arrow_statistics.rs:229:9
6: parquet_exec::parquet::arrow_statistics::test_data_page_stats_with_all_null_page::{{closure}}
at ./tests/parquet/arrow_statistics.rs:567:9
7: <core::pin::Pin<P> as core::future::future::Future>::poll
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/future/future.rs:123:9
8: <core::pin::Pin<P> as core::future::future::Future>::poll
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/future/future.rs:123:9
9: tokio::runtime::scheduler::current_thread::CoreGuard::block_on::{{closure}}::{{closure}}::{{closure}}
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:659:57
10: tokio::runtime::coop::with_budget
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/coop.rs:107:5
11: tokio::runtime::coop::budget
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/coop.rs:73:5
12: tokio::runtime::scheduler::current_thread::CoreGuard::block_on::{{closure}}::{{closure}}
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:659:25
13: tokio::runtime::scheduler::current_thread::Context::enter
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:404:19
14: tokio::runtime::scheduler::current_thread::CoreGuard::block_on::{{closure}}
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:658:36
15: tokio::runtime::scheduler::current_thread::CoreGuard::enter::{{closure}}
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:737:68
16: tokio::runtime::context::scoped::Scoped<T>::set
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/context/scoped.rs:40:9
17: tokio::runtime::context::set_scheduler::{{closure}}
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/context.rs:180:26
18: std::thread::local::LocalKey<T>::try_with
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/thread/local.rs:286:12
19: std::thread::local::LocalKey<T>::with
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/thread/local.rs:262:9
20: tokio::runtime::context::set_scheduler
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/context.rs:180:9
21: tokio::runtime::scheduler::current_thread::CoreGuard::enter
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:737:27
22: tokio::runtime::scheduler::current_thread::CoreGuard::block_on
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:646:19
23: tokio::runtime::scheduler::current_thread::CurrentThread::block_on::{{closure}}
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:175:28
24: tokio::runtime::context::runtime::enter_runtime
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/context/runtime.rs:65:16
25: tokio::runtime::scheduler::current_thread::CurrentThread::block_on
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:167:9
26: tokio::runtime::runtime::Runtime::block_on
at /Users/andrewlamb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/runtime.rs:347:47
27: parquet_exec::parquet::arrow_statistics::test_data_page_stats_with_all_null_page
at ./tests/parquet/arrow_statistics.rs:517:5
28: parquet_exec::parquet::arrow_statistics::test_data_page_stats_with_all_null_page::{{closure}}
at ./tests/parquet/arrow_statistics.rs:516:51
29: core::ops::function::FnOnce::call_once
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/ops/function.rs:250:5
30: core::ops::function::FnOnce::call_once
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
```
--
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 data page statistics when all rows are null in a data page [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11295:
URL: https://github.com/apache/datafusion/pull/11295#issuecomment-2212585423
THanks again@
--
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 data page statistics when all rows are null in a data page [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #11295:
URL: https://github.com/apache/datafusion/pull/11295
--
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 data page statistics when all rows are null in a data page [datafusion]
Posted by "efredine (via GitHub)" <gi...@apache.org>.
efredine commented on code in PR #11295:
URL: https://github.com/apache/datafusion/pull/11295#discussion_r1667766119
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -823,11 +819,11 @@ macro_rules! get_data_page_statistics {
Float16Array::from_iter(
[<$stat_type_prefix Float16DataPageStatsIterator>]::new($iterator)
.map(|x| {
- x.into_iter().filter_map(|x| {
- x.and_then(|x| Some(from_bytes_to_f16(x.data())))
+ x.into_iter().map(|x| {
+ x.and_then(|x| from_bytes_to_f16(x.data()))
})
})
- .flatten()
+ .flatten().collect::<Vec<_>>()
Review Comment:
Good catch - thanks.
--
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 data page statistics when all rows are null in a data page [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11295:
URL: https://github.com/apache/datafusion/pull/11295#issuecomment-2212585411
Let's merge this one in so we can proceed with getting https://github.com/apache/datafusion/pull/11319 ready
--
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