You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "marvinlanhenke (via GitHub)" <gi...@apache.org> on 2024/06/17 11:22:12 UTC
[PR] Add bench for data page statistics parquet extraction [datafusion]
marvinlanhenke opened a new pull request, #10950:
URL: https://github.com/apache/datafusion/pull/10950
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #10934.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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] Add bench for data page statistics parquet extraction [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10950:
URL: https://github.com/apache/datafusion/pull/10950#issuecomment-2174359210
Thanks again @marvinlanhenke -- this PR looks good to me. Per your suggestion, let's wait until the required type support has been added
--
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] Add bench for data page statistics parquet extraction [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10950:
URL: https://github.com/apache/datafusion/pull/10950#discussion_r1642804890
##########
datafusion/core/benches/parquet_statistic.rs:
##########
@@ -82,11 +103,18 @@ fn create_parquet_file(dtype: TestTypes, row_groups: usize) -> NamedTempFile {
for _ in 0..row_groups {
let batch = match dtype {
TestTypes::UInt64 => make_uint64_batch(),
+ TestTypes::Int64 => make_int64_batch(),
TestTypes::F64 => make_f64_batch(),
TestTypes::String => make_string_batch(),
TestTypes::Dictionary => make_dict_batch(),
};
- writer.write(&batch).unwrap();
+ if data_page_row_count_limit.is_some() {
Review Comment:
Perhaps we can add a comment here explaining why we write a batch at a time (basically because the page limit is only checked on record batch boundaries
##########
datafusion/core/benches/parquet_statistic.rs:
##########
@@ -70,7 +84,14 @@ fn create_parquet_file(dtype: TestTypes, row_groups: usize) -> NamedTempFile {
)])),
};
- let props = WriterProperties::builder().build();
+ let mut props = WriterProperties::builder();
Review Comment:
🤔 while looking at this code, It seems like the benchmark for row groups doesn't actually make multiple row groups (it instead it encodes `row_goups` batches instead)
We should probably also set https://docs.rs/parquet/latest/parquet/file/properties/struct.WriterProperties.html#method.max_row_group_size as well 🤔
##########
datafusion/core/benches/parquet_statistic.rs:
##########
@@ -150,37 +195,93 @@ fn make_dict_batch() -> RecordBatch {
fn criterion_benchmark(c: &mut Criterion) {
let row_groups = 100;
use TestTypes::*;
- let types = vec![UInt64, F64, String, Dictionary];
+ let types = vec![Int64, UInt64, F64, String, Dictionary];
+ let data_page_row_count_limits = vec![None, Some(1)];
for dtype in types {
- let file = create_parquet_file(dtype.clone(), row_groups);
- let file = file.reopen().unwrap();
- let reader = ArrowReaderBuilder::try_new(file).unwrap();
- let metadata = reader.metadata();
- let row_groups = metadata.row_groups();
-
- let mut group =
- c.benchmark_group(format!("Extract statistics for {}", dtype.clone()));
- group.bench_function(
- BenchmarkId::new("extract_statistics", dtype.clone()),
- |b| {
- b.iter(|| {
- let converter = StatisticsConverter::try_new(
- "col",
- reader.schema(),
- reader.parquet_schema(),
- )
- .unwrap();
-
- let _ = converter.row_group_mins(row_groups.iter()).unwrap();
- let _ = converter.row_group_maxes(row_groups.iter()).unwrap();
- let _ = converter.row_group_null_counts(row_groups.iter()).unwrap();
- let _ = StatisticsConverter::row_group_row_counts(row_groups.iter())
+ for data_page_row_count_limit in &data_page_row_count_limits {
+ let file =
+ create_parquet_file(dtype.clone(), row_groups, data_page_row_count_limit);
+ let file = file.reopen().unwrap();
+ let options = ArrowReaderOptions::new().with_page_index(true);
+ let reader = ArrowReaderBuilder::try_new_with_options(file, options).unwrap();
+ let metadata = reader.metadata();
+ let row_groups = metadata.row_groups();
+ let row_group_indices = row_groups
Review Comment:
I think we could get the correct indices using the following potentially shorter code:
```rust
let row_group_indices: Vec<_> = (0..row_groups.len()).collect();
```
--
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] Add bench for data page statistics parquet extraction [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #10950:
URL: https://github.com/apache/datafusion/pull/10950#issuecomment-2173563137
> The other thing we could do is somehow ignore those columns until the support has been added
yes, but this would require another PR to revert back those changes. I think adding the bench is not so urgent; so merging once other dataypes are ready might be the easiest thing to do.
I've also addressed your other comments; should be fine now. Thanks for the review.
--
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] Add bench for data page statistics parquet extraction [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10950:
URL: https://github.com/apache/datafusion/pull/10950#discussion_r1642902984
##########
datafusion/core/benches/parquet_statistic.rs:
##########
@@ -70,7 +84,14 @@ fn create_parquet_file(dtype: TestTypes, row_groups: usize) -> NamedTempFile {
)])),
};
- let props = WriterProperties::builder().build();
+ let mut props = WriterProperties::builder();
Review Comment:
i fixed this. however, it should not make any difference since we have it set at `100` but only have 5 rows in our batch
--
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] Add bench for data page statistics parquet extraction [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10950:
URL: https://github.com/apache/datafusion/pull/10950#issuecomment-2207303315
Thanks for checking this out @efredine -- I merged up from main and updated this PR to get it moving. Once it passes CI I think it is good to go
Thanks again @marvinlanhenke
Like @efredine I verified the benchmark now works without error:
```shell
cargo bench --bench parquet_statistic
Compiling bigdecimal v0.4.1
Compiling datafusion v39.0.0 (/Users/andrewlamb/Software/datafusion2/datafusion/core)
Finished `bench` profile [optimized] target(s) in 1m 34s
Running benches/parquet_statistic.rs (target/release/deps/parquet_statistic-c6fce472dea5abe8)
Gnuplot not found, using plotters backend
Extract row group statistics for Int64/extract_statistics/Int64
time: [594.98 ns 596.23 ns 597.66 ns]
Found 10 outliers among 100 measurements (10.00%)
4 (4.00%) high mild
6 (6.00%) high severe
Extract data page statistics for Int64/extract_statistics/Int64
time: [6.5665 µs 6.5848 µs 6.6047 µs]
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe
Extract row group statistics for UInt64/extract_statistics/UInt64
time: [576.78 ns 578.78 ns 581.09 ns]
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe
Extract data page statistics for UInt64/extract_statistics/UInt64
time: [6.8120 µs 6.8332 µs 6.8559 µs]
Found 7 outliers among 100 measurements (7.00%)
6 (6.00%) high mild
1 (1.00%) high severe
Extract row group statistics for F64/extract_statistics/F64
time: [588.96 ns 592.68 ns 596.62 ns]
Extract data page statistics for F64/extract_statistics/F64
time: [7.5959 µs 7.6334 µs 7.6650 µs]
Extract row group statistics for String/extract_statistics/String
time: [897.07 ns 901.70 ns 907.19 ns]
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe
Extract data page statistics for String/extract_statistics/String
time: [25.507 µs 25.555 µs 25.609 µs]
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe
Benchmarking Extract row group statistics for Dictionary(Int32, String)/extract_statistics/Dictionary(Int32, Stri...: Collecting 100 samples in estimated 5.00
Extract row group statistics for Dictionary(Int32, String)/extract_statistics/Dictionary(Int32, Stri...
time: [947.78 ns 954.30 ns 960.82 ns]
Found 8 outliers among 100 measurements (8.00%)
1 (1.00%) low severe
7 (7.00%) low mild
Benchmarking Extract data page statistics for Dictionary(Int32, String)/extract_statistics/Dictionary(Int32, Stri...: Collecting 100 samples in estimated 5.04
Extract data page statistics for Dictionary(Int32, String)/extract_statistics/Dictionary(Int32, Stri...
time: [25.602 µs 25.812 µs 26.109 µs]
Found 8 outliers among 100 measurements (8.00%)
7 (7.00%) high mild
1 (1.00%) high severe
```
--
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] Add bench for data page statistics parquet extraction [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10950:
URL: https://github.com/apache/datafusion/pull/10950#issuecomment-2173377872
> Should we simply wait until the missing data types are supported and then merge this PR?
I think that makes sense to me
--
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] Add bench for data page statistics parquet extraction [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #10950:
URL: https://github.com/apache/datafusion/pull/10950#issuecomment-2173130658
@alamb
...since not all DataTypes are supported, some types panic in the bench.
Should we simply wait until the missing data types are supported and then merge this PR?
--
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] Add bench for data page statistics parquet extraction [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10950:
URL: https://github.com/apache/datafusion/pull/10950
--
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