You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "gabotechs (via GitHub)" <gi...@apache.org> on 2026/02/11 15:44:45 UTC
[PR] Add statistics integration tests [datafusion]
gabotechs opened a new pull request, #20292:
URL: https://github.com/apache/datafusion/pull/20292
## 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.
-->
TODO: create an epic for stats estimation improvements
- Closes #.
## Rationale for this change
The statistics propagation system is lacking some overall accuracy. There should be many low hanging fruits regarding statistic propagation that if addressed could increase the quality of the physical plans as a results of better estimations.
The idea behind this PR is to add some tests for qualifying further improvements in the statistic propagation system, so that people can incrementally improve it over time.
## What changes are included in this PR?
Adds some new integration tests that compare the results estimated through statistics vs what actually happened within the query for the TPC-DS dataset (just that one for now):
1. It calls `ExecutePlan::partition_statistics()` on all the nodes, collecting their estimations.
2. It executes the plan against actual parquet files, collecting the relevant metrics.
3. It computes an accuracy factor based on what was estimated VS what actually happen
4. It commits the avg accuracy into insta snapshots in the tests, so that further contributions can show some improvements in the overall accuracy number.
It also adds a `Debug` implementation that allows visualizing the comparison in a fine-grained way:
```
SortPreservingMergeExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
SortExec(TopK): output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
ProjectionExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
AggregateExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
RepartitionExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
AggregateExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
RepartitionExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
HashJoinExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
HashJoinExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
HashJoinExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
CoalescePartitionsExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
HashJoinExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
HashJoinExec: output_bytes=Absent vs 0 (0%) output_bytes=Inexact(1) vs 0 (0%)
CoalescePartitionsExec: output_bytes=Inexact(12) vs 32832 (1%) output_bytes=Inexact(1) vs 1 (100%)
ProjectionExec: output_bytes=Inexact(12) vs 32832 (1%) output_bytes=Inexact(1) vs 1 (100%)
FilterExec: output_bytes=Inexact(8) vs 32768 (1%) output_bytes=Inexact(1) vs 1 (100%)
RepartitionExec: output_bytes=Inexact(12000) vs 98304 (13%) output_bytes=Inexact(1000) vs 63 (7%)
DataSourceExec: output_bytes=Inexact(12000) vs 756 (7%) output_bytes=Inexact(1000) vs 63 (7%)
DataSourceExec: output_bytes=Inexact(44000) vs 67520 (66%) output_bytes=Inexact(1000) vs 1000 (100%)
FilterExec: output_bytes=Absent vs 442368 (0%) output_bytes=Inexact(1) vs 21 (5%)
RepartitionExec: output_bytes=Absent vs 525095 (0%) output_bytes=Inexact(1000) vs 1000 (100%)
DataSourceExec: output_bytes=Absent vs 96717 (0%) output_bytes=Inexact(1000) vs 1000 (100%)
DataSourceExec: output_bytes=Inexact(16000) vs 16064 (100%) output_bytes=Inexact(1000) vs 1000 (100%)
DataSourceExec: output_bytes=Absent vs 44589 (0%) output_bytes=Inexact(1000) vs 1000 (100%)
DataSourceExec: output_bytes=Absent vs 450 (0%) output_bytes=Inexact(12) vs 12 (100%)
```
## Are these changes tested?
This changes are exclusively new integration 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 statistics integration tests [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2817398909
##########
datafusion/core/tests/statistics/tpcds.rs:
##########
@@ -0,0 +1,876 @@
+// 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.
+
+#![allow(clippy::absurd_extreme_comparisons)]
+#![allow(unused_comparisons)]
+
+use crate::statistics::StatsVsMetricsDisplayOptions;
+use datafusion::common::Result;
+use datafusion::prelude::{ParquetReadOptions, SessionContext};
+use datafusion_physical_plan::collect;
+use std::fs;
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::statistics::Node;
+
+ #[tokio::test]
+ async fn tpcds_1() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(1).await?;
+ assert!(result.row_estimation_accuracy >= 20);
Review Comment:
Unfortunately, unlike other languages like Golang or JS, Rust does not play well with that pattern, as you would:
1. Lose individual test reporting to stdout or in IDE tools.
2. Lose the ability to hook up a debugger for one specific test case.
Test cases in Rust are defined by their literal function names, so Cargo tooling expects you to put individual test cases in different functions.
You can see that this is actually the pattern the rest of the DataFusion codebase is using (all I've checked at least)
--
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 statistics integration tests [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-3911205795
> I'm looking at that document, but it does not seem to refer to any tpcds files already created in this project. It seems like the TPC-DS files are present in a different project https://github.com/apache/datafusion-benchmarks, which I don't think the CI or the tests clone automatically before running the tests.
Sorry, Ive lost this thread. Actually before running TPCDS queries the runner clones the repo. There was another activity which Im' struggling to find right now where TPCDS would be generated automatically the same way TPCH does. Once its done we can get rid of predefined TPCDS files from benchmarks repo
--
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 statistics integration tests [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-4034316553
> > How about using tpchgen-rs to generate them instead of checking them in?
>
> Those are TPCDS
Despite the name, `tpchgen-cli` now has support for TPCDS now:
- https://github.com/clflushopt/tpchgen-rs/pull/212
Though I am not sure we have released any version with this yet 🤔
--
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 statistics integration tests [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-4037988529
> @alamb do you see any way we can have NDV/distinct_count when generating test files via that tool?
>
> There has been a lot of activity and interest lately around NDV (including myself), it'd be nice to be able to track progress via this new test suite.
@asolimando I think we would need to add support to arrow-rs for writing the distinct statistics first
- https://github.com/apache/arrow-rs/issues/8608
One challenge we have had so far is that distinct count is often quite memory and CPU expensive to calculate (there are less intensive estimates too like hyper log log but the actual parquet statistics call for NDV).
--
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 statistics integration tests [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-4473683032
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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 statistics integration tests [datafusion]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-4034610176
> > > How about using tpchgen-rs to generate them instead of checking them in?
> >
> >
> > Those are TPCDS
>
> Despite the name, `tpchgen-cli` now has support for TPCDS now:
>
> * [feat: TPC-DS support clflushopt/tpchgen-rs#212](https://github.com/clflushopt/tpchgen-rs/pull/212)
>
> Though I am not sure we have released any version with this yet 🤔
@alamb do you see any way we can have NDV/distinct_count when generating test files via that tool?
There has been a lot of activity and interest lately around NDV (including myself), it'd be nice to be able to track progress via this new test suite.
--
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 statistics integration tests [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-3912578114
Note that these are also not the normal TPC-DS files, they having been sampled with https://github.com/xitongsys/parquet-go/tree/master/tool/parquet-tools so that they are smaller and suitable for unit testing. The approach I've followed is the same already existed with TPC-H in this integration tests: to commit a reduced version of the files, like https://github.com/apache/datafusion/blob/main/datafusion/core/tests/data/tpch_lineitem_small.parquet.
--
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 statistics integration tests [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2820725593
##########
datafusion/core/tests/statistics/mod.rs:
##########
@@ -0,0 +1,161 @@
+// 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 datafusion::common::Result;
+use datafusion::physical_plan::ExecutionPlan;
+use datafusion_common::Statistics;
+use datafusion_common::stats::Precision;
+use datafusion_physical_expr_common::metrics::MetricValue;
+use std::fmt::{Debug, Display, Formatter};
+use std::sync::Arc;
+
+mod tpcds;
+
+#[derive(Debug, Default, Copy, Clone)]
+struct StatsVsMetricsDisplayOptions {
+ display_output_rows: bool,
+ display_output_bytes: bool,
+}
+
+struct Node {
+ name: String,
+ stats: Statistics,
+ output_rows: Option<usize>,
+ output_bytes: Option<usize>,
+ children: Vec<Node>,
+ opts: StatsVsMetricsDisplayOptions,
+}
+
+impl Debug for Node {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ fn fmt(f: &mut Formatter<'_>, node: &Node, depth: usize) -> std::fmt::Result {
+ for _ in 0..depth {
+ write!(f, " ")?;
+ }
+ write!(f, "{}:", node.name)?;
+ fn display_opt<T: Display>(opt: Option<T>) -> impl Display {
+ match opt {
+ None => "?".to_string(),
+ Some(v) => v.to_string(),
+ }
+ }
+ if node.opts.display_output_rows {
+ write!(
+ f,
+ " output_rows={} vs {} ({}%)",
+ node.stats.num_rows,
+ display_opt(node.output_rows),
+ display_opt(accuracy_percent(node.stats.num_rows, node.output_rows))
+ )?;
+ }
+ if node.opts.display_output_bytes {
+ write!(
+ f,
+ " output_bytes={} vs {} ({}%)",
+ node.stats.total_byte_size,
+ display_opt(node.output_bytes),
+ display_opt(accuracy_percent(
+ node.stats.total_byte_size,
+ node.output_bytes
+ ))
+ )?;
+ }
+ writeln!(f)?;
+ for c in &node.children {
+ fmt(f, c, depth + 1)?;
+ }
+ Ok(())
+ }
+
+ fmt(f, self, 0)
+ }
+}
+
+impl Node {
+ fn from_plan(
+ plan: &Arc<dyn ExecutionPlan>,
+ opts: StatsVsMetricsDisplayOptions,
+ ) -> Result<Self> {
+ let mut children = vec![];
+ for child in plan.children() {
+ children.push(Node::from_plan(child, opts)?);
+ }
+
+ let mut node = Node {
+ name: plan.name().to_string(),
+ stats: plan.partition_statistics(None)?,
+ output_rows: None,
+ output_bytes: None,
+ children,
+ opts,
+ };
+ if let Some(metrics) = plan.metrics() {
+ node.output_rows = metrics.output_rows();
+ node.output_bytes = metrics
+ .sum(|v| matches!(v.value(), MetricValue::OutputBytes(_)))
+ .map(|v| v.as_usize());
+ }
+
+ Ok(node)
+ }
+
+ fn avg_row_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.num_rows, node.output_rows)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
+ }
+
+ fn avg_byte_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.total_byte_size, node.output_bytes)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
+ }
+}
+
+fn accuracy_percent(estimated: Precision<usize>, actual: Option<usize>) -> Option<usize> {
+ match (estimated.get_value(), actual) {
+ (Some(estimated), Some(actual)) => {
+ let err = (100 * estimated.abs_diff(actual)) / actual.max(*estimated).max(1);
+ Some(100 - err)
+ }
+ (Some(_estimated), None) => None,
Review Comment:
In `(Some(_estimated), None)`, the right `None` does not mean that no rows where returned by that node, it means that the node is just not measuring how many rows flowed through it, just because whoever authored it did not implement it. What I imagine is that we cannot draw any conclusions in that situation.
--
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 statistics integration tests [datafusion]
Posted by "ruchirK (via GitHub)" <gi...@apache.org>.
ruchirK commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2840982023
##########
datafusion/core/tests/statistics/mod.rs:
##########
@@ -0,0 +1,161 @@
+// 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 datafusion::common::Result;
+use datafusion::physical_plan::ExecutionPlan;
+use datafusion_common::Statistics;
+use datafusion_common::stats::Precision;
+use datafusion_physical_expr_common::metrics::MetricValue;
+use std::fmt::{Debug, Display, Formatter};
+use std::sync::Arc;
+
+mod tpcds;
+
+#[derive(Debug, Default, Copy, Clone)]
+struct StatsVsMetricsDisplayOptions {
+ display_output_rows: bool,
+ display_output_bytes: bool,
+}
+
+struct Node {
+ name: String,
+ stats: Statistics,
+ output_rows: Option<usize>,
+ output_bytes: Option<usize>,
+ children: Vec<Node>,
+ opts: StatsVsMetricsDisplayOptions,
+}
+
+impl Debug for Node {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ fn fmt(f: &mut Formatter<'_>, node: &Node, depth: usize) -> std::fmt::Result {
+ for _ in 0..depth {
+ write!(f, " ")?;
+ }
+ write!(f, "{}:", node.name)?;
+ fn display_opt<T: Display>(opt: Option<T>) -> impl Display {
+ match opt {
+ None => "?".to_string(),
+ Some(v) => v.to_string(),
+ }
+ }
+ if node.opts.display_output_rows {
+ write!(
+ f,
+ " output_rows={} vs {} ({}%)",
+ node.stats.num_rows,
+ display_opt(node.output_rows),
+ display_opt(accuracy_percent(node.stats.num_rows, node.output_rows))
+ )?;
+ }
+ if node.opts.display_output_bytes {
+ write!(
+ f,
+ " output_bytes={} vs {} ({}%)",
+ node.stats.total_byte_size,
+ display_opt(node.output_bytes),
+ display_opt(accuracy_percent(
+ node.stats.total_byte_size,
+ node.output_bytes
+ ))
+ )?;
+ }
+ writeln!(f)?;
+ for c in &node.children {
+ fmt(f, c, depth + 1)?;
+ }
+ Ok(())
+ }
+
+ fmt(f, self, 0)
+ }
+}
+
+impl Node {
+ fn from_plan(
+ plan: &Arc<dyn ExecutionPlan>,
+ opts: StatsVsMetricsDisplayOptions,
+ ) -> Result<Self> {
+ let mut children = vec![];
+ for child in plan.children() {
+ children.push(Node::from_plan(child, opts)?);
+ }
+
+ let mut node = Node {
+ name: plan.name().to_string(),
+ stats: plan.partition_statistics(None)?,
+ output_rows: None,
+ output_bytes: None,
+ children,
+ opts,
+ };
+ if let Some(metrics) = plan.metrics() {
+ node.output_rows = metrics.output_rows();
+ node.output_bytes = metrics
+ .sum(|v| matches!(v.value(), MetricValue::OutputBytes(_)))
+ .map(|v| v.as_usize());
+ }
+
+ Ok(node)
+ }
+
+ fn avg_row_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.num_rows, node.output_rows)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
+ }
+
+ fn avg_byte_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.total_byte_size, node.output_bytes)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
+ }
+}
+
+fn accuracy_percent(estimated: Precision<usize>, actual: Option<usize>) -> Option<usize> {
+ match (estimated.get_value(), actual) {
+ (Some(estimated), Some(actual)) => {
+ let err = (100 * estimated.abs_diff(actual)) / actual.max(*estimated).max(1);
+ Some(100 - err)
+ }
+ (Some(_estimated), None) => None,
Review Comment:
that makes sense. I think its fine as is but I think we have some kinda small risk here in that if we return None for all the nodes in the plan we then get a div by zero when we divide by accuracy.len() if im understanding it right
I think maybe a different question here is - do we want all nodes to measure how many rows flow through them? if so, a different candidate for the return value is `Some(50)` - if we really can't say anything about the number of rows then potentially every error value is equally likely, and so in expectation the error is 50%? and having a nonzero error will help drive all nodes towards implementing the stats. If we don't want all nodes to measure stats then I fully agree returning None seems great
--
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 statistics integration tests [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2794045455
##########
datafusion/core/tests/data/tpcds_catalog_returns_small.parquet:
##########
Review Comment:
I'm not sure if this sizes are acceptable for this project. The new parquet files are the original ones for the TPC-DS benchmark sampled to 1000 rows max.
The total of all the files is 1.8Mb, usually I'd consider this acceptable to be committed directly to a git repository, but let me know if this falls within this project's policy.
--
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 statistics integration tests [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-4033441694
> @alamb WDYT of having another copy of TCPDS small parquet files in the repo? Reg to [#18985 (review)](https://github.com/apache/datafusion/pull/18985#pullrequestreview-3536054691) there was a concern of having diff TPCDS queries for testing and benchmarks, here we have the same situation of having diff TPCDS data files for testing and benchmarks
How about using tpchgen-rs to generate them instead of checking them in?
--
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 statistics integration tests [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-4033546738
>
> How about using tpchgen-rs to generate them instead of checking them in?
Those are TPCDS
--
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 statistics integration tests [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2955369282
##########
datafusion/core/tests/statistics/tpcds.rs:
##########
@@ -0,0 +1,876 @@
+// 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.
+
+#![allow(clippy::absurd_extreme_comparisons)]
+#![allow(unused_comparisons)]
+
+use crate::statistics::StatsVsMetricsDisplayOptions;
+use datafusion::common::Result;
+use datafusion::prelude::{ParquetReadOptions, SessionContext};
+use datafusion_physical_plan::collect;
+use std::fs;
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::statistics::Node;
+
+ #[tokio::test]
+ async fn tpcds_1() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(1).await?;
+ assert!(result.row_estimation_accuracy >= 20);
+ assert!(result.byte_estimation_accuracy >= 8);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_2() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(2).await?;
+ assert!(result.row_estimation_accuracy >= 34);
+ assert!(result.byte_estimation_accuracy >= 16);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_3() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(3).await?;
+ assert!(result.row_estimation_accuracy >= 44);
+ assert!(result.byte_estimation_accuracy >= 10);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_4() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(4).await?;
+ assert!(result.row_estimation_accuracy >= 22);
+ assert!(result.byte_estimation_accuracy >= 4);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_5() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(5).await?;
+ assert!(result.row_estimation_accuracy >= 21);
+ assert!(result.byte_estimation_accuracy >= 12);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_6() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(6).await?;
+ assert!(result.row_estimation_accuracy >= 28);
+ assert!(result.byte_estimation_accuracy >= 5);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_7() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(7).await?;
+ assert!(result.row_estimation_accuracy >= 34);
+ assert!(result.byte_estimation_accuracy >= 4);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_8() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(8).await?;
+ assert!(result.row_estimation_accuracy >= 34);
+ assert!(result.byte_estimation_accuracy >= 2);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_9() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(9).await?;
+ assert!(result.row_estimation_accuracy >= 55);
+ assert!(result.byte_estimation_accuracy >= 30);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_10() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(10).await?;
+ assert!(result.row_estimation_accuracy >= 33);
+ assert!(result.byte_estimation_accuracy >= 7);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_11() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(11).await?;
+ assert!(result.row_estimation_accuracy >= 23);
+ assert!(result.byte_estimation_accuracy >= 6);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_12() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(12).await?;
+ assert!(result.row_estimation_accuracy >= 22);
+ assert!(result.byte_estimation_accuracy >= 5);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_13() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(13).await?;
+ assert!(result.row_estimation_accuracy >= 54);
+ assert!(result.byte_estimation_accuracy >= 9);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_14() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(14).await?;
+ assert!(result.row_estimation_accuracy >= 37);
+ assert!(result.byte_estimation_accuracy >= 16);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_15() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(15).await?;
+ assert!(result.row_estimation_accuracy >= 15);
+ assert!(result.byte_estimation_accuracy >= 8);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_16() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(16).await?;
+ assert!(result.row_estimation_accuracy >= 24);
+ assert!(result.byte_estimation_accuracy >= 9);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_17() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(17).await?;
+ assert!(result.row_estimation_accuracy >= 11);
+ assert!(result.byte_estimation_accuracy >= 6);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_18() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(18).await?;
+ assert!(result.row_estimation_accuracy >= 36);
+ assert!(result.byte_estimation_accuracy >= 6);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_19() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(19).await?;
+ assert!(result.row_estimation_accuracy >= 36);
+ assert!(result.byte_estimation_accuracy >= 7);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_20() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(20).await?;
+ assert!(result.row_estimation_accuracy >= 22);
+ assert!(result.byte_estimation_accuracy >= 5);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_21() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(21).await?;
+ assert!(result.row_estimation_accuracy >= 31);
+ assert!(result.byte_estimation_accuracy >= 3);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_22() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(22).await?;
+ assert!(result.row_estimation_accuracy >= 18);
+ assert!(result.byte_estimation_accuracy >= 2);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_23() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(23).await?;
+ assert!(result.row_estimation_accuracy >= 18);
+ assert!(result.byte_estimation_accuracy >= 12);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_24() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(24).await?;
+ assert!(result.row_estimation_accuracy >= 33);
+ assert!(result.byte_estimation_accuracy >= 7);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_25() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(25).await?;
+ assert!(result.row_estimation_accuracy >= 18);
+ assert!(result.byte_estimation_accuracy >= 6);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_26() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(26).await?;
+ assert!(result.row_estimation_accuracy >= 31);
+ assert!(result.byte_estimation_accuracy >= 2);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_27() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(27).await?;
+ assert!(result.row_estimation_accuracy >= 20);
+ assert!(result.byte_estimation_accuracy >= 3);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_28() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(28).await?;
+ assert!(result.row_estimation_accuracy >= 72);
+ assert!(result.byte_estimation_accuracy >= 39);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_29() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(29).await?;
+ assert!(result.row_estimation_accuracy >= 25);
+ assert!(result.byte_estimation_accuracy >= 8);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_30() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(30).await?;
+ assert!(result.row_estimation_accuracy >= 26);
+ assert!(result.byte_estimation_accuracy >= 3);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_31() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(31).await?;
+ assert!(result.row_estimation_accuracy >= 18);
+ assert!(result.byte_estimation_accuracy >= 6);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_32() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(32).await?;
+ assert!(result.row_estimation_accuracy >= 27);
+ assert!(result.byte_estimation_accuracy >= 13);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_33() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(33).await?;
+ assert!(result.row_estimation_accuracy >= 41);
+ assert!(result.byte_estimation_accuracy >= 12);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_34() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(34).await?;
+ assert!(result.row_estimation_accuracy >= 17);
+ assert!(result.byte_estimation_accuracy >= 3);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_35() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(35).await?;
+ assert!(result.row_estimation_accuracy >= 33);
+ assert!(result.byte_estimation_accuracy >= 8);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_36() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(36).await?;
+ assert!(result.row_estimation_accuracy >= 10);
+ assert!(result.byte_estimation_accuracy >= 3);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_37() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(37).await?;
+ assert!(result.row_estimation_accuracy >= 27);
+ assert!(result.byte_estimation_accuracy >= 9);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_38() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(38).await?;
+ assert!(result.row_estimation_accuracy >= 27);
+ assert!(result.byte_estimation_accuracy >= 7);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_39() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(39).await?;
+ assert!(result.row_estimation_accuracy >= 15);
+ assert!(result.byte_estimation_accuracy >= 9);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_40() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(40).await?;
+ assert!(result.row_estimation_accuracy >= 35);
+ assert!(result.byte_estimation_accuracy >= 9);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_41() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(41).await?;
+ assert!(result.row_estimation_accuracy >= 33);
+ assert!(result.byte_estimation_accuracy >= 0);
Review Comment:
Let's try to come up with a way of removing this assertions
##########
datafusion/core/tests/statistics/mod.rs:
##########
@@ -0,0 +1,161 @@
+// 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 datafusion::common::Result;
+use datafusion::physical_plan::ExecutionPlan;
+use datafusion_common::Statistics;
+use datafusion_common::stats::Precision;
+use datafusion_physical_expr_common::metrics::MetricValue;
+use std::fmt::{Debug, Display, Formatter};
+use std::sync::Arc;
+
+mod tpcds;
+
+#[derive(Debug, Default, Copy, Clone)]
+struct StatsVsMetricsDisplayOptions {
+ display_output_rows: bool,
+ display_output_bytes: bool,
+}
+
+struct Node {
+ name: String,
+ stats: Statistics,
+ output_rows: Option<usize>,
+ output_bytes: Option<usize>,
+ children: Vec<Node>,
+ opts: StatsVsMetricsDisplayOptions,
+}
+
+impl Debug for Node {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ fn fmt(f: &mut Formatter<'_>, node: &Node, depth: usize) -> std::fmt::Result {
+ for _ in 0..depth {
+ write!(f, " ")?;
+ }
+ write!(f, "{}:", node.name)?;
+ fn display_opt<T: Display>(opt: Option<T>) -> impl Display {
+ match opt {
+ None => "?".to_string(),
+ Some(v) => v.to_string(),
+ }
+ }
+ if node.opts.display_output_rows {
+ write!(
+ f,
+ " output_rows={} vs {} ({}%)",
+ node.stats.num_rows,
+ display_opt(node.output_rows),
+ display_opt(accuracy_percent(node.stats.num_rows, node.output_rows))
+ )?;
+ }
+ if node.opts.display_output_bytes {
+ write!(
+ f,
+ " output_bytes={} vs {} ({}%)",
+ node.stats.total_byte_size,
+ display_opt(node.output_bytes),
+ display_opt(accuracy_percent(
+ node.stats.total_byte_size,
+ node.output_bytes
+ ))
+ )?;
+ }
+ writeln!(f)?;
+ for c in &node.children {
+ fmt(f, c, depth + 1)?;
+ }
+ Ok(())
+ }
+
+ fmt(f, self, 0)
+ }
+}
+
+impl Node {
+ fn from_plan(
+ plan: &Arc<dyn ExecutionPlan>,
+ opts: StatsVsMetricsDisplayOptions,
+ ) -> Result<Self> {
+ let mut children = vec![];
+ for child in plan.children() {
+ children.push(Node::from_plan(child, opts)?);
+ }
+
+ let mut node = Node {
+ name: plan.name().to_string(),
+ stats: plan.partition_statistics(None)?,
+ output_rows: None,
+ output_bytes: None,
+ children,
+ opts,
+ };
+ if let Some(metrics) = plan.metrics() {
+ node.output_rows = metrics.output_rows();
+ node.output_bytes = metrics
+ .sum(|v| matches!(v.value(), MetricValue::OutputBytes(_)))
+ .map(|v| v.as_usize());
+ }
+
+ Ok(node)
+ }
+
+ fn avg_row_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.num_rows, node.output_rows)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
+ }
+
+ fn avg_byte_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.total_byte_size, node.output_bytes)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
Review Comment:
Same as above.
--
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 statistics integration tests [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-4013327609
@alamb WDYT of having another copy of TCPDS small parquet files in the repo? Reg to https://github.com/apache/datafusion/pull/18985#pullrequestreview-3536054691 there was a concern of having diff TPCDS queries for testing and benchmarks, here we have the same situation of having diff TPCDS data files for testing and 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 statistics integration tests [datafusion]
Posted by "xudong963 (via GitHub)" <gi...@apache.org>.
xudong963 commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2916947709
##########
datafusion/core/tests/statistics/mod.rs:
##########
@@ -0,0 +1,161 @@
+// 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 datafusion::common::Result;
+use datafusion::physical_plan::ExecutionPlan;
+use datafusion_common::Statistics;
+use datafusion_common::stats::Precision;
+use datafusion_physical_expr_common::metrics::MetricValue;
+use std::fmt::{Debug, Display, Formatter};
+use std::sync::Arc;
+
+mod tpcds;
+
+#[derive(Debug, Default, Copy, Clone)]
+struct StatsVsMetricsDisplayOptions {
+ display_output_rows: bool,
+ display_output_bytes: bool,
+}
+
+struct Node {
+ name: String,
+ stats: Statistics,
+ output_rows: Option<usize>,
+ output_bytes: Option<usize>,
+ children: Vec<Node>,
+ opts: StatsVsMetricsDisplayOptions,
+}
+
+impl Debug for Node {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ fn fmt(f: &mut Formatter<'_>, node: &Node, depth: usize) -> std::fmt::Result {
+ for _ in 0..depth {
+ write!(f, " ")?;
+ }
+ write!(f, "{}:", node.name)?;
+ fn display_opt<T: Display>(opt: Option<T>) -> impl Display {
+ match opt {
+ None => "?".to_string(),
+ Some(v) => v.to_string(),
+ }
+ }
+ if node.opts.display_output_rows {
+ write!(
+ f,
+ " output_rows={} vs {} ({}%)",
+ node.stats.num_rows,
+ display_opt(node.output_rows),
+ display_opt(accuracy_percent(node.stats.num_rows, node.output_rows))
+ )?;
+ }
+ if node.opts.display_output_bytes {
+ write!(
+ f,
+ " output_bytes={} vs {} ({}%)",
+ node.stats.total_byte_size,
+ display_opt(node.output_bytes),
+ display_opt(accuracy_percent(
+ node.stats.total_byte_size,
+ node.output_bytes
+ ))
+ )?;
+ }
+ writeln!(f)?;
+ for c in &node.children {
+ fmt(f, c, depth + 1)?;
+ }
+ Ok(())
+ }
+
+ fmt(f, self, 0)
+ }
+}
+
+impl Node {
+ fn from_plan(
+ plan: &Arc<dyn ExecutionPlan>,
+ opts: StatsVsMetricsDisplayOptions,
+ ) -> Result<Self> {
+ let mut children = vec![];
+ for child in plan.children() {
+ children.push(Node::from_plan(child, opts)?);
+ }
+
+ let mut node = Node {
+ name: plan.name().to_string(),
+ stats: plan.partition_statistics(None)?,
+ output_rows: None,
+ output_bytes: None,
+ children,
+ opts,
+ };
+ if let Some(metrics) = plan.metrics() {
+ node.output_rows = metrics.output_rows();
+ node.output_bytes = metrics
+ .sum(|v| matches!(v.value(), MetricValue::OutputBytes(_)))
+ .map(|v| v.as_usize());
+ }
+
+ Ok(node)
+ }
+
+ fn avg_row_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.num_rows, node.output_rows)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
+ }
+
+ fn avg_byte_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.total_byte_size, node.output_bytes)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
Review Comment:
is it possible that `accuracy.len()` is zero?
##########
datafusion/core/tests/statistics/tpcds.rs:
##########
@@ -0,0 +1,876 @@
+// 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.
+
+#![allow(clippy::absurd_extreme_comparisons)]
+#![allow(unused_comparisons)]
+
+use crate::statistics::StatsVsMetricsDisplayOptions;
+use datafusion::common::Result;
+use datafusion::prelude::{ParquetReadOptions, SessionContext};
+use datafusion_physical_plan::collect;
+use std::fs;
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::statistics::Node;
+
+ #[tokio::test]
+ async fn tpcds_1() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(1).await?;
+ assert!(result.row_estimation_accuracy >= 20);
+ assert!(result.byte_estimation_accuracy >= 8);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_2() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(2).await?;
+ assert!(result.row_estimation_accuracy >= 34);
+ assert!(result.byte_estimation_accuracy >= 16);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_3() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(3).await?;
+ assert!(result.row_estimation_accuracy >= 44);
+ assert!(result.byte_estimation_accuracy >= 10);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_4() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(4).await?;
+ assert!(result.row_estimation_accuracy >= 22);
+ assert!(result.byte_estimation_accuracy >= 4);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_5() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(5).await?;
+ assert!(result.row_estimation_accuracy >= 21);
+ assert!(result.byte_estimation_accuracy >= 12);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_6() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(6).await?;
+ assert!(result.row_estimation_accuracy >= 28);
+ assert!(result.byte_estimation_accuracy >= 5);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_7() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(7).await?;
+ assert!(result.row_estimation_accuracy >= 34);
+ assert!(result.byte_estimation_accuracy >= 4);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_8() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(8).await?;
+ assert!(result.row_estimation_accuracy >= 34);
+ assert!(result.byte_estimation_accuracy >= 2);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_9() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(9).await?;
+ assert!(result.row_estimation_accuracy >= 55);
+ assert!(result.byte_estimation_accuracy >= 30);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_10() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(10).await?;
+ assert!(result.row_estimation_accuracy >= 33);
+ assert!(result.byte_estimation_accuracy >= 7);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_11() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(11).await?;
+ assert!(result.row_estimation_accuracy >= 23);
+ assert!(result.byte_estimation_accuracy >= 6);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_12() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(12).await?;
+ assert!(result.row_estimation_accuracy >= 22);
+ assert!(result.byte_estimation_accuracy >= 5);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_13() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(13).await?;
+ assert!(result.row_estimation_accuracy >= 54);
+ assert!(result.byte_estimation_accuracy >= 9);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_14() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(14).await?;
+ assert!(result.row_estimation_accuracy >= 37);
+ assert!(result.byte_estimation_accuracy >= 16);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_15() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(15).await?;
+ assert!(result.row_estimation_accuracy >= 15);
+ assert!(result.byte_estimation_accuracy >= 8);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_16() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(16).await?;
+ assert!(result.row_estimation_accuracy >= 24);
+ assert!(result.byte_estimation_accuracy >= 9);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_17() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(17).await?;
+ assert!(result.row_estimation_accuracy >= 11);
+ assert!(result.byte_estimation_accuracy >= 6);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_18() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(18).await?;
+ assert!(result.row_estimation_accuracy >= 36);
+ assert!(result.byte_estimation_accuracy >= 6);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_19() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(19).await?;
+ assert!(result.row_estimation_accuracy >= 36);
+ assert!(result.byte_estimation_accuracy >= 7);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_20() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(20).await?;
+ assert!(result.row_estimation_accuracy >= 22);
+ assert!(result.byte_estimation_accuracy >= 5);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_21() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(21).await?;
+ assert!(result.row_estimation_accuracy >= 31);
+ assert!(result.byte_estimation_accuracy >= 3);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_22() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(22).await?;
+ assert!(result.row_estimation_accuracy >= 18);
+ assert!(result.byte_estimation_accuracy >= 2);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_23() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(23).await?;
+ assert!(result.row_estimation_accuracy >= 18);
+ assert!(result.byte_estimation_accuracy >= 12);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_24() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(24).await?;
+ assert!(result.row_estimation_accuracy >= 33);
+ assert!(result.byte_estimation_accuracy >= 7);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_25() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(25).await?;
+ assert!(result.row_estimation_accuracy >= 18);
+ assert!(result.byte_estimation_accuracy >= 6);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_26() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(26).await?;
+ assert!(result.row_estimation_accuracy >= 31);
+ assert!(result.byte_estimation_accuracy >= 2);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_27() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(27).await?;
+ assert!(result.row_estimation_accuracy >= 20);
+ assert!(result.byte_estimation_accuracy >= 3);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_28() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(28).await?;
+ assert!(result.row_estimation_accuracy >= 72);
+ assert!(result.byte_estimation_accuracy >= 39);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_29() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(29).await?;
+ assert!(result.row_estimation_accuracy >= 25);
+ assert!(result.byte_estimation_accuracy >= 8);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_30() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(30).await?;
+ assert!(result.row_estimation_accuracy >= 26);
+ assert!(result.byte_estimation_accuracy >= 3);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_31() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(31).await?;
+ assert!(result.row_estimation_accuracy >= 18);
+ assert!(result.byte_estimation_accuracy >= 6);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_32() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(32).await?;
+ assert!(result.row_estimation_accuracy >= 27);
+ assert!(result.byte_estimation_accuracy >= 13);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_33() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(33).await?;
+ assert!(result.row_estimation_accuracy >= 41);
+ assert!(result.byte_estimation_accuracy >= 12);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_34() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(34).await?;
+ assert!(result.row_estimation_accuracy >= 17);
+ assert!(result.byte_estimation_accuracy >= 3);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_35() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(35).await?;
+ assert!(result.row_estimation_accuracy >= 33);
+ assert!(result.byte_estimation_accuracy >= 8);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_36() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(36).await?;
+ assert!(result.row_estimation_accuracy >= 10);
+ assert!(result.byte_estimation_accuracy >= 3);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_37() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(37).await?;
+ assert!(result.row_estimation_accuracy >= 27);
+ assert!(result.byte_estimation_accuracy >= 9);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_38() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(38).await?;
+ assert!(result.row_estimation_accuracy >= 27);
+ assert!(result.byte_estimation_accuracy >= 7);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_39() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(39).await?;
+ assert!(result.row_estimation_accuracy >= 15);
+ assert!(result.byte_estimation_accuracy >= 9);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_40() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(40).await?;
+ assert!(result.row_estimation_accuracy >= 35);
+ assert!(result.byte_estimation_accuracy >= 9);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn tpcds_41() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(41).await?;
+ assert!(result.row_estimation_accuracy >= 33);
+ assert!(result.byte_estimation_accuracy >= 0);
Review Comment:
this assert is alway true, right?
--
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 statistics integration tests [datafusion]
Posted by "ruchirK (via GitHub)" <gi...@apache.org>.
ruchirK commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2817767571
##########
datafusion/core/tests/statistics/mod.rs:
##########
@@ -0,0 +1,161 @@
+// 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 datafusion::common::Result;
+use datafusion::physical_plan::ExecutionPlan;
+use datafusion_common::Statistics;
+use datafusion_common::stats::Precision;
+use datafusion_physical_expr_common::metrics::MetricValue;
+use std::fmt::{Debug, Display, Formatter};
+use std::sync::Arc;
+
+mod tpcds;
+
+#[derive(Debug, Default, Copy, Clone)]
+struct StatsVsMetricsDisplayOptions {
+ display_output_rows: bool,
+ display_output_bytes: bool,
+}
+
+struct Node {
+ name: String,
+ stats: Statistics,
+ output_rows: Option<usize>,
+ output_bytes: Option<usize>,
+ children: Vec<Node>,
+ opts: StatsVsMetricsDisplayOptions,
+}
+
+impl Debug for Node {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ fn fmt(f: &mut Formatter<'_>, node: &Node, depth: usize) -> std::fmt::Result {
+ for _ in 0..depth {
+ write!(f, " ")?;
+ }
+ write!(f, "{}:", node.name)?;
+ fn display_opt<T: Display>(opt: Option<T>) -> impl Display {
+ match opt {
+ None => "?".to_string(),
+ Some(v) => v.to_string(),
+ }
+ }
+ if node.opts.display_output_rows {
+ write!(
+ f,
+ " output_rows={} vs {} ({}%)",
+ node.stats.num_rows,
+ display_opt(node.output_rows),
+ display_opt(accuracy_percent(node.stats.num_rows, node.output_rows))
+ )?;
+ }
+ if node.opts.display_output_bytes {
+ write!(
+ f,
+ " output_bytes={} vs {} ({}%)",
+ node.stats.total_byte_size,
+ display_opt(node.output_bytes),
+ display_opt(accuracy_percent(
+ node.stats.total_byte_size,
+ node.output_bytes
+ ))
+ )?;
+ }
+ writeln!(f)?;
+ for c in &node.children {
+ fmt(f, c, depth + 1)?;
+ }
+ Ok(())
+ }
+
+ fmt(f, self, 0)
+ }
+}
+
+impl Node {
+ fn from_plan(
+ plan: &Arc<dyn ExecutionPlan>,
+ opts: StatsVsMetricsDisplayOptions,
+ ) -> Result<Self> {
+ let mut children = vec![];
+ for child in plan.children() {
+ children.push(Node::from_plan(child, opts)?);
+ }
+
+ let mut node = Node {
+ name: plan.name().to_string(),
+ stats: plan.partition_statistics(None)?,
+ output_rows: None,
+ output_bytes: None,
+ children,
+ opts,
+ };
+ if let Some(metrics) = plan.metrics() {
+ node.output_rows = metrics.output_rows();
+ node.output_bytes = metrics
+ .sum(|v| matches!(v.value(), MetricValue::OutputBytes(_)))
+ .map(|v| v.as_usize());
+ }
+
+ Ok(node)
+ }
+
+ fn avg_row_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.num_rows, node.output_rows)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
+ }
+
+ fn avg_byte_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.total_byte_size, node.output_bytes)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
+ }
+}
+
+fn accuracy_percent(estimated: Precision<usize>, actual: Option<usize>) -> Option<usize> {
+ match (estimated.get_value(), actual) {
+ (Some(estimated), Some(actual)) => {
+ let err = (100 * estimated.abs_diff(actual)) / actual.max(*estimated).max(1);
+ Some(100 - err)
+ }
+ (Some(_estimated), None) => None,
Review Comment:
I'm curious why if we have a nonzero `_estimated` we don't wanna report this as 100% error (which we would if `actual` was 0 instead of None)?
--
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 statistics integration tests [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2794051511
##########
datafusion/core/tests/statistics/tpcds.rs:
##########
@@ -0,0 +1,1087 @@
+// 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 crate::statistics::StatsVsMetricsDisplayOptions;
+use datafusion::common::Result;
+use datafusion::prelude::{ParquetReadOptions, SessionContext};
+use datafusion_physical_plan::collect;
+use std::fs;
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::statistics::Node;
+ use insta::assert_snapshot;
+ use std::fmt::{Display, Formatter};
+
+ #[tokio::test]
+ async fn tpcds_1() -> Result<()> {
+ let display = tpcds_stats_vs_metrics_display(1).await?;
+ assert_snapshot!(display, @r"
+ row_estimation_accuracy=20%
+ byte_estimation_accuracy=8%
+ ");
+ Ok(())
Review Comment:
The idea is that further contributions should see this numbers getting increased.
--
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 statistics integration tests [datafusion]
Posted by "ruchirK (via GitHub)" <gi...@apache.org>.
ruchirK commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2817356079
##########
datafusion/core/tests/statistics/tpcds.rs:
##########
@@ -0,0 +1,876 @@
+// 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.
+
+#![allow(clippy::absurd_extreme_comparisons)]
+#![allow(unused_comparisons)]
+
+use crate::statistics::StatsVsMetricsDisplayOptions;
+use datafusion::common::Result;
+use datafusion::prelude::{ParquetReadOptions, SessionContext};
+use datafusion_physical_plan::collect;
+use std::fs;
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::statistics::Node;
+
+ #[tokio::test]
+ async fn tpcds_1() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(1).await?;
+ assert!(result.row_estimation_accuracy >= 20);
Review Comment:
non-blocking / nit: you could potentially consider having all of these test cases just be written down as an array of
[
(tpdcds_number, row_estimation_accuracy, byte_estimation_accuracy),
...
]
and then a for loop that iterates through all of them and does the checks. I feel like that might be a more concise test/make patterns in the data more obvious, but im not sure if the runtime of that one test would then be overly long because now individual tests can't run in parallel (you could mitigate by spawning each future in parallel and then joining on the whole thing but maybe thats not worth the complexity)
--
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 statistics integration tests [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #20292: Add statistics integration tests
URL: https://github.com/apache/datafusion/pull/20292
--
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 statistics integration tests [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2955284599
##########
datafusion/core/tests/statistics/mod.rs:
##########
@@ -0,0 +1,161 @@
+// 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 datafusion::common::Result;
+use datafusion::physical_plan::ExecutionPlan;
+use datafusion_common::Statistics;
+use datafusion_common::stats::Precision;
+use datafusion_physical_expr_common::metrics::MetricValue;
+use std::fmt::{Debug, Display, Formatter};
+use std::sync::Arc;
+
+mod tpcds;
+
+#[derive(Debug, Default, Copy, Clone)]
+struct StatsVsMetricsDisplayOptions {
+ display_output_rows: bool,
+ display_output_bytes: bool,
+}
+
+struct Node {
+ name: String,
+ stats: Statistics,
+ output_rows: Option<usize>,
+ output_bytes: Option<usize>,
+ children: Vec<Node>,
+ opts: StatsVsMetricsDisplayOptions,
+}
+
+impl Debug for Node {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ fn fmt(f: &mut Formatter<'_>, node: &Node, depth: usize) -> std::fmt::Result {
+ for _ in 0..depth {
+ write!(f, " ")?;
+ }
+ write!(f, "{}:", node.name)?;
+ fn display_opt<T: Display>(opt: Option<T>) -> impl Display {
+ match opt {
+ None => "?".to_string(),
+ Some(v) => v.to_string(),
+ }
+ }
+ if node.opts.display_output_rows {
+ write!(
+ f,
+ " output_rows={} vs {} ({}%)",
+ node.stats.num_rows,
+ display_opt(node.output_rows),
+ display_opt(accuracy_percent(node.stats.num_rows, node.output_rows))
+ )?;
+ }
+ if node.opts.display_output_bytes {
+ write!(
+ f,
+ " output_bytes={} vs {} ({}%)",
+ node.stats.total_byte_size,
+ display_opt(node.output_bytes),
+ display_opt(accuracy_percent(
+ node.stats.total_byte_size,
+ node.output_bytes
+ ))
+ )?;
+ }
+ writeln!(f)?;
+ for c in &node.children {
+ fmt(f, c, depth + 1)?;
+ }
+ Ok(())
+ }
+
+ fmt(f, self, 0)
+ }
+}
+
+impl Node {
Review Comment:
Good point, done!
--
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 statistics integration tests [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-4084665684
@xudong963 I in fact do not love the idea of computing the accuracy scores as I'm doing now, what I would have liked to do is to instead use `insta` assertions over the debug representation of the stats comparison. You have one example on the PR description.
This is how I would have liked to do it https://github.com/apache/datafusion/pull/20292/commits/e912551bd2cd6d6089741036288fa91a78991097.
It's a verbose assertion, but a pretty granular and useful one. The issue is that the results seem to be machine dependent: executing the tests on my Mac consistently passes the assertions, however, the same tests run on linux show significantly different results, so the CI will fail.
--
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 statistics integration tests [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-3895565661
> Thanks @gabotechs I think you can use already created tpcds files, please refer to https://github.com/apache/datafusion/blob/main/benchmarks/README.md#tpcds-1
I'm looking at that document, but it does not seem to refer to any tpcds files already created in this project. It seems like the TPC-DS files are present in a different project https://github.com/apache/datafusion-benchmarks, which I don't think the CI or the tests clone automatically before running the tests.
--
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 statistics integration tests [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2845000962
##########
datafusion/core/tests/statistics/mod.rs:
##########
@@ -0,0 +1,161 @@
+// 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 datafusion::common::Result;
+use datafusion::physical_plan::ExecutionPlan;
+use datafusion_common::Statistics;
+use datafusion_common::stats::Precision;
+use datafusion_physical_expr_common::metrics::MetricValue;
+use std::fmt::{Debug, Display, Formatter};
+use std::sync::Arc;
+
+mod tpcds;
+
+#[derive(Debug, Default, Copy, Clone)]
+struct StatsVsMetricsDisplayOptions {
+ display_output_rows: bool,
+ display_output_bytes: bool,
+}
+
+struct Node {
+ name: String,
+ stats: Statistics,
+ output_rows: Option<usize>,
+ output_bytes: Option<usize>,
+ children: Vec<Node>,
+ opts: StatsVsMetricsDisplayOptions,
+}
+
+impl Debug for Node {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ fn fmt(f: &mut Formatter<'_>, node: &Node, depth: usize) -> std::fmt::Result {
+ for _ in 0..depth {
+ write!(f, " ")?;
+ }
+ write!(f, "{}:", node.name)?;
+ fn display_opt<T: Display>(opt: Option<T>) -> impl Display {
+ match opt {
+ None => "?".to_string(),
+ Some(v) => v.to_string(),
+ }
+ }
+ if node.opts.display_output_rows {
+ write!(
+ f,
+ " output_rows={} vs {} ({}%)",
+ node.stats.num_rows,
+ display_opt(node.output_rows),
+ display_opt(accuracy_percent(node.stats.num_rows, node.output_rows))
+ )?;
+ }
+ if node.opts.display_output_bytes {
+ write!(
+ f,
+ " output_bytes={} vs {} ({}%)",
+ node.stats.total_byte_size,
+ display_opt(node.output_bytes),
+ display_opt(accuracy_percent(
+ node.stats.total_byte_size,
+ node.output_bytes
+ ))
+ )?;
+ }
+ writeln!(f)?;
+ for c in &node.children {
+ fmt(f, c, depth + 1)?;
+ }
+ Ok(())
+ }
+
+ fmt(f, self, 0)
+ }
+}
+
+impl Node {
+ fn from_plan(
+ plan: &Arc<dyn ExecutionPlan>,
+ opts: StatsVsMetricsDisplayOptions,
+ ) -> Result<Self> {
+ let mut children = vec![];
+ for child in plan.children() {
+ children.push(Node::from_plan(child, opts)?);
+ }
+
+ let mut node = Node {
+ name: plan.name().to_string(),
+ stats: plan.partition_statistics(None)?,
+ output_rows: None,
+ output_bytes: None,
+ children,
+ opts,
+ };
+ if let Some(metrics) = plan.metrics() {
+ node.output_rows = metrics.output_rows();
+ node.output_bytes = metrics
+ .sum(|v| matches!(v.value(), MetricValue::OutputBytes(_)))
+ .map(|v| v.as_usize());
+ }
+
+ Ok(node)
+ }
+
+ fn avg_row_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.num_rows, node.output_rows)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
+ }
+
+ fn avg_byte_accuracy(&self) -> usize {
+ fn collect_accuracy(node: &Node) -> Vec<usize> {
+ let mut results = vec![];
+ for child in &node.children {
+ results.extend(collect_accuracy(child));
+ }
+ if let Some(accuracy) =
+ accuracy_percent(node.stats.total_byte_size, node.output_bytes)
+ {
+ results.push(accuracy);
+ }
+ results
+ }
+ let accuracy = collect_accuracy(self);
+ accuracy.iter().sum::<usize>() / accuracy.len()
+ }
+}
+
+fn accuracy_percent(estimated: Precision<usize>, actual: Option<usize>) -> Option<usize> {
+ match (estimated.get_value(), actual) {
+ (Some(estimated), Some(actual)) => {
+ let err = (100 * estimated.abs_diff(actual)) / actual.max(*estimated).max(1);
+ Some(100 - err)
+ }
+ (Some(_estimated), None) => None,
Review Comment:
Pretty much all nodes in DataFusion are already measuring the output rows. There's a mechanism in place for doing it pretty automatically, so I think it's fair to assume that all nodes measure output rows.
It's also fine to get panics because of div by zero, here, as this will only run in tests.
--
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 statistics integration tests [datafusion]
Posted by "ruchirK (via GitHub)" <gi...@apache.org>.
ruchirK commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2817700199
##########
datafusion/core/tests/statistics/tpcds.rs:
##########
@@ -0,0 +1,876 @@
+// 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.
+
+#![allow(clippy::absurd_extreme_comparisons)]
+#![allow(unused_comparisons)]
+
+use crate::statistics::StatsVsMetricsDisplayOptions;
+use datafusion::common::Result;
+use datafusion::prelude::{ParquetReadOptions, SessionContext};
+use datafusion_physical_plan::collect;
+use std::fs;
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::statistics::Node;
+
+ #[tokio::test]
+ async fn tpcds_1() -> Result<()> {
+ let result = tpcds_stats_vs_metrics(1).await?;
+ assert!(result.row_estimation_accuracy >= 20);
Review Comment:
ok makes sense! ive been in python too long and all i can think of is pytest :)
--
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 statistics integration tests [datafusion]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#issuecomment-3985410056
I just realized that the parquet files are missing the `distinct_count` statistics, would it be possible to updated them to add it?
I am asking this with #19957 in mind (and future NDV-based improvements), it would be nice to be able to measure the accuracy improvement from NDV-aware cardinality estimation directly when this PR gets merged.
Of course it's fine to post-pone this in a follow-up PR, asking in case you agree it's interesting and not too much work.
(IIRC duckdb allows to write parquet files with `distinct_count` set)
--
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 statistics integration tests [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20292:
URL: https://github.com/apache/datafusion/pull/20292#discussion_r2897092130
##########
datafusion/core/tests/statistics/mod.rs:
##########
@@ -0,0 +1,161 @@
+// 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 datafusion::common::Result;
+use datafusion::physical_plan::ExecutionPlan;
+use datafusion_common::Statistics;
+use datafusion_common::stats::Precision;
+use datafusion_physical_expr_common::metrics::MetricValue;
+use std::fmt::{Debug, Display, Formatter};
+use std::sync::Arc;
+
+mod tpcds;
+
+#[derive(Debug, Default, Copy, Clone)]
+struct StatsVsMetricsDisplayOptions {
+ display_output_rows: bool,
+ display_output_bytes: bool,
+}
+
+struct Node {
+ name: String,
+ stats: Statistics,
+ output_rows: Option<usize>,
+ output_bytes: Option<usize>,
+ children: Vec<Node>,
+ opts: StatsVsMetricsDisplayOptions,
+}
+
+impl Debug for Node {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ fn fmt(f: &mut Formatter<'_>, node: &Node, depth: usize) -> std::fmt::Result {
+ for _ in 0..depth {
+ write!(f, " ")?;
+ }
+ write!(f, "{}:", node.name)?;
+ fn display_opt<T: Display>(opt: Option<T>) -> impl Display {
+ match opt {
+ None => "?".to_string(),
+ Some(v) => v.to_string(),
+ }
+ }
+ if node.opts.display_output_rows {
+ write!(
+ f,
+ " output_rows={} vs {} ({}%)",
+ node.stats.num_rows,
+ display_opt(node.output_rows),
+ display_opt(accuracy_percent(node.stats.num_rows, node.output_rows))
+ )?;
+ }
+ if node.opts.display_output_bytes {
+ write!(
+ f,
+ " output_bytes={} vs {} ({}%)",
+ node.stats.total_byte_size,
+ display_opt(node.output_bytes),
+ display_opt(accuracy_percent(
+ node.stats.total_byte_size,
+ node.output_bytes
+ ))
+ )?;
+ }
+ writeln!(f)?;
+ for c in &node.children {
+ fmt(f, c, depth + 1)?;
+ }
+ Ok(())
+ }
+
+ fmt(f, self, 0)
+ }
+}
+
+impl Node {
Review Comment:
nit: we should add some comments for clarity + better developer experience
--
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