Repository navigation
refactor: deprecate the bundled statistics providers #25969
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2f5a6e0
7a6782c
5c9b6b0
2b3c10d
b74c6b5
7811713
45db23b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ use datafusion::catalog::{ | |
| CatalogProvider, MemoryCatalogProvider, MemorySchemaProvider, SchemaProvider, Session, | ||
| }; | ||
| use datafusion::common::config::Dialect; | ||
| use datafusion::common::stats::Precision; | ||
| use datafusion::common::{DataFusionError, Result, not_impl_err}; | ||
| use datafusion::functions::math::abs; | ||
| use datafusion::logical_expr::async_udf::{AsyncScalarUDF, AsyncScalarUDFImpl}; | ||
|
|
@@ -63,7 +64,11 @@ use datafusion::common::cast::as_float64_array; | |
| use datafusion::execution::SessionStateBuilder; | ||
| use datafusion::execution::memory_pool::UnboundedMemoryPool; | ||
| use datafusion::execution::runtime_env::RuntimeEnvBuilder; | ||
| use datafusion::physical_plan::operator_statistics::StatisticsRegistry; | ||
| use datafusion::physical_plan::joins::HashJoinExec; | ||
| use datafusion::physical_plan::operator_statistics::{ | ||
| ClosureStatisticsProvider, StatisticsRegistry, StatisticsResult, | ||
| }; | ||
| use datafusion::physical_plan::statistics::StatisticsArgs; | ||
| use log::info; | ||
| use sqlparser::ast; | ||
| use tempfile::TempDir; | ||
|
|
@@ -144,9 +149,31 @@ impl TestContext { | |
| relative_path.file_name().and_then(|name| name.to_str()), | ||
| Some("statistics_registry.slt") | ||
| ) { | ||
| state_builder = state_builder.with_statistics_registry( | ||
| StatisticsRegistry::default_with_builtin_providers(), | ||
| // Replaces the join estimate with the Cartesian product | ||
| let join_provider = ClosureStatisticsProvider::with_matches( | ||
| |plan| plan.downcast_ref::<HashJoinExec>().is_some(), | ||
|
Comment on lines
+152
to
+154
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see this is still using a If it's just that, all good, but let me know if missed something.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I trimmed the example down to exactly that, just testing the registry works, you got it right |
||
| |plan, child_stats| { | ||
| let (Some(&left_rows), Some(&right_rows)) = ( | ||
| child_stats[0].base().num_rows.get_value(), | ||
| child_stats[1].base().num_rows.get_value(), | ||
| ) else { | ||
| return Ok(StatisticsResult::Delegate); | ||
| }; | ||
| let child_base = child_stats | ||
| .iter() | ||
| .map(|c| Arc::clone(c.base_arc())) | ||
| .collect::<Vec<_>>(); | ||
| let mut stats = Arc::unwrap_or_clone( | ||
| plan.statistics_from_inputs(&child_base, &StatisticsArgs::new())?, | ||
| ); | ||
| stats.num_rows = | ||
| Precision::Inexact(left_rows.saturating_mul(right_rows)); | ||
| Ok(StatisticsResult::Computed(stats.into())) | ||
| }, | ||
| ); | ||
| let registry = | ||
| StatisticsRegistry::with_providers(vec![Arc::new(join_provider)]); | ||
| state_builder = state_builder.with_statistics_registry(registry); | ||
| } | ||
|
|
||
| let state = state_builder.build(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks good. Not sure which tickets are already open for this, but what comes to mind is:
partition_statisticsmethod.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two follow-up issues are listed here, which follow your proposal 1. to fold improvements in the built-in estimation.
For 2. I have filed #26055 following https://datafusion.apache.org/contributor-guide/api-health.html#deprecation-guidelines so we won't forget.