You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "dhegberg (via GitHub)" <gi...@apache.org> on 2024/11/24 03:26:14 UTC
[PR] Add csv loading benchmarks. [datafusion]
dhegberg opened a new pull request, #13544:
URL: https://github.com/apache/datafusion/pull/13544
## Which issue does this PR close?
Related to #12904
## Rationale for this change
Requested in comments for `https://github.com/apache/datafusion/pull/13228`
A direct testing on loading csv files was identified as a gap in the benchmarking suite.
## What changes are included in this PR?
Basic benchmarks related to loading csv files.
## Are these changes tested?
Tested via `./bench.sh run csv`
Logged output:
```
Running csv load benchmarks.
Generated test dataset with 10240283 rows
Executing 'CSV Load Speed Test.'
Iteration 0 finished in 7.079167 ms.
Iteration 1 finished in 3.3643750000000003 ms.
Iteration 2 finished in 3.2645 ms.
Iteration 3 finished in 3.311208 ms.
Iteration 4 finished in 3.319 ms.
Done
```
results file:
[csv.json](https://github.com/user-attachments/files/17891638/csv.json)
Curious result is the first iteration is consistently 6-7 ms vs ~3ms on future iterations. Is a new `SessionContext` not sufficient to remove any cache in loading?
<!--
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?
No
<!--
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 csv loading benchmarks. [datafusion]
Posted by "dhegberg (via GitHub)" <gi...@apache.org>.
dhegberg commented on PR #13544:
URL: https://github.com/apache/datafusion/pull/13544#issuecomment-2513566309
Updated benchmarks output:
```
☁ core [csv_benchmark] cargo bench --jobs 2 --bench csv_load -- --verbose
Finished `bench` profile [optimized] target(s) in 0.19s
Running benches/csv_load.rs (/Users/dhegberg/workplace/datafusion/target/release/deps/csv_load-bd58bc3aea1aaed0)
Gnuplot not found, using plotters backend
Generated test dataset with 69642 rows
Benchmarking load csv testing/default csv read options
Benchmarking load csv testing/default csv read options: Warming up for 3.0000 s
Benchmarking load csv testing/default csv read options: Collecting 100 samples in estimated 20.197 s (1100 iterations)
Benchmarking load csv testing/default csv read options: Analyzing
load csv testing/default csv read options
time: [20.094 ms 20.263 ms 20.426 ms]
change: [-0.5031% +0.9097% +2.1820%] (p = 0.20 > 0.05)
No change in performance detected.
mean [20.094 ms 20.426 ms] std. dev. [729.56 µs 941.81 µs]
median [20.314 ms 20.622 ms] med. abs. dev. [440.35 µs 1.0435 ms]
```
--
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 csv loading benchmarks. [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #13544:
URL: https://github.com/apache/datafusion/pull/13544#discussion_r1868872032
##########
datafusion/core/benches/csv_load.rs:
##########
@@ -0,0 +1,81 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#[macro_use]
+extern crate criterion;
+extern crate arrow;
+extern crate datafusion;
+
+mod data_utils;
+use crate::criterion::Criterion;
+use datafusion::error::Result;
+use datafusion::execution::context::SessionContext;
+use datafusion::prelude::CsvReadOptions;
+use datafusion::test_util::csv::TestCsvFile;
+use parking_lot::Mutex;
+use std::sync::Arc;
+use std::time::Duration;
+use test_utils::AccessLogGenerator;
+use tokio::runtime::Runtime;
+
+fn load_csv(ctx: Arc<Mutex<SessionContext>>, path: &str, options: CsvReadOptions) {
+ let rt = Runtime::new().unwrap();
Review Comment:
Is it better to give this as a parameter and to create it outside of the measurement?
--
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 csv loading benchmarks. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13544:
URL: https://github.com/apache/datafusion/pull/13544#discussion_r1870585570
##########
datafusion/core/benches/csv_load.rs:
##########
@@ -0,0 +1,81 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#[macro_use]
+extern crate criterion;
+extern crate arrow;
+extern crate datafusion;
+
+mod data_utils;
+use crate::criterion::Criterion;
+use datafusion::error::Result;
+use datafusion::execution::context::SessionContext;
+use datafusion::prelude::CsvReadOptions;
+use datafusion::test_util::csv::TestCsvFile;
+use parking_lot::Mutex;
+use std::sync::Arc;
+use std::time::Duration;
+use test_utils::AccessLogGenerator;
+use tokio::runtime::Runtime;
+
+fn load_csv(ctx: Arc<Mutex<SessionContext>>, path: &str, options: CsvReadOptions) {
+ let rt = Runtime::new().unwrap();
Review Comment:
I think keeping consistent with the other benchmarks is preferable
--
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 csv loading benchmarks. [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #13544:
URL: https://github.com/apache/datafusion/pull/13544#issuecomment-2498574013
> I don't have a strong opinion on the location of the benchmarks, so I'm happy to follow recommendations.
>
> For my future reference, how do you differentiate this functionality vs. the parquet related testing in the top level benchmarks?
I don't want to misguide you. Perhaps @alamb can direct you better for that.
--
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 csv loading benchmarks. [datafusion]
Posted by "dhegberg (via GitHub)" <gi...@apache.org>.
dhegberg commented on PR #13544:
URL: https://github.com/apache/datafusion/pull/13544#issuecomment-2514955171
@berkaysynnada @alamb
I've moved the benchmarks and they're ready for 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 csv loading benchmarks. [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #13544:
URL: https://github.com/apache/datafusion/pull/13544#issuecomment-2497175722
Hi @dhegberg, thank you for your contribution—it’s well-written and formatted nicely. We have a dedicated path for operator-specific benchmarks: https://github.com/apache/datafusion/tree/main/datafusion/physical-plan/benches. It seems to me that the measured functionality falls under the scope of `CsvExec`. Do you think it would be better to include these benchmarks there?
--
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 csv loading benchmarks. [datafusion]
Posted by "dhegberg (via GitHub)" <gi...@apache.org>.
dhegberg commented on PR #13544:
URL: https://github.com/apache/datafusion/pull/13544#issuecomment-2498306483
I don't have a strong opinion on the location of the benchmarks, so I'm happy to follow recommendations.
For my future reference, how do you differentiate this functionality vs. the parquet related testing in the top level benchmarks?
--
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 csv loading benchmarks. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13544:
URL: https://github.com/apache/datafusion/pull/13544
--
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 csv loading benchmarks. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13544:
URL: https://github.com/apache/datafusion/pull/13544#issuecomment-2504613315
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another 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] Add csv loading benchmarks. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13544:
URL: https://github.com/apache/datafusion/pull/13544#discussion_r1857348347
##########
benchmarks/src/csv/load.rs:
##########
@@ -0,0 +1,75 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::path::PathBuf;
+
+use crate::util::{BenchmarkRun, CommonOpt};
+use datafusion::{
+ common::Result,
+ prelude::{CsvReadOptions, SessionContext},
+};
+
+use datafusion_common::instant::Instant;
+use structopt::StructOpt;
+
+use super::data::DataOpt;
+
+#[derive(Debug, StructOpt, Clone)]
+#[structopt(verbatim_doc_comment)]
+pub struct RunOpt {
+ /// Common options
+ #[structopt(flatten)]
+ common: CommonOpt,
+
+ /// Create data files
+ #[structopt(flatten)]
+ data: DataOpt,
+
+ /// Path to machine readable output file
+ #[structopt(parse(from_os_str), short = "o", long = "output")]
+ output_path: Option<PathBuf>,
+}
+
+impl RunOpt {
+ pub async fn run(self) -> Result<()> {
+ let test_file = self.data.build()?;
+ let mut rundata = BenchmarkRun::new();
Review Comment:
One thing I would like to request is that we split the data generation from the query.
Given this setup, rerunning the benchmarks will likely be dominated by the time it takes to regenerate the input which will be quite slow
--
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 csv loading benchmarks. [datafusion]
Posted by "dhegberg (via GitHub)" <gi...@apache.org>.
dhegberg commented on code in PR #13544:
URL: https://github.com/apache/datafusion/pull/13544#discussion_r1870014671
##########
datafusion/core/benches/csv_load.rs:
##########
@@ -0,0 +1,81 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#[macro_use]
+extern crate criterion;
+extern crate arrow;
+extern crate datafusion;
+
+mod data_utils;
+use crate::criterion::Criterion;
+use datafusion::error::Result;
+use datafusion::execution::context::SessionContext;
+use datafusion::prelude::CsvReadOptions;
+use datafusion::test_util::csv::TestCsvFile;
+use parking_lot::Mutex;
+use std::sync::Arc;
+use std::time::Duration;
+use test_utils::AccessLogGenerator;
+use tokio::runtime::Runtime;
+
+fn load_csv(ctx: Arc<Mutex<SessionContext>>, path: &str, options: CsvReadOptions) {
+ let rt = Runtime::new().unwrap();
Review Comment:
It seems that this is consistent with the other benchmarks in this package, Runtime is initialized within the bench function iterator.
I can change it here though if you think it should be initialized before hand.
--
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