-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(pruning): expose IN-list rewrite size cap as a config option #24074
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
base: main
Are you sure you want to change the base?
Changes from all commits
98e7a5a
bb42e89
5f4cfa1
4ad8d50
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 |
|---|---|---|
|
|
@@ -1189,6 +1189,22 @@ config_namespace! { | |
| /// parquet reader setting. 0 means no caching. | ||
| pub max_predicate_cache_size: Option<usize>, default = None | ||
|
|
||
| /// Maximum number of values in an `IN (...)` list for which the | ||
| /// pruning predicate will rewrite the list into a chain of per-value | ||
| /// statistics checks. Lists longer than this fall back to the | ||
| /// unhandled-predicate hook (defaulting to "keep the container"), | ||
| /// which effectively skips container-level pruning for large IN | ||
| /// lists. | ||
| /// | ||
| /// Higher values keep row-group / file-range statistics pruning | ||
| /// effective for larger IN lists (for example, REST endpoints that | ||
| /// filter by a batch of ~25-100 identifiers), at the cost of a | ||
| /// larger rewritten predicate expression evaluated for every | ||
| /// container. Set to 0 to disable the rewrite path entirely. | ||
| /// | ||
| /// The default of 20 preserves the previous hardcoded behaviour. | ||
| pub pruning_max_in_list_size: usize, default = 20 | ||
|
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. Also I suggest changing this to be something more conisstent with the others names like Perhaps something like |
||
|
|
||
| // The following options affect writing to parquet files | ||
| // and map to parquet::file::properties::WriterProperties | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,9 @@ use datafusion_physical_expr_common::sort_expr::LexOrdering; | |
| use datafusion_physical_plan::metrics::{ | ||
| BaselineMetrics, Count, ExecutionPlanMetricsSet, MetricBuilder, MetricCategory, | ||
| }; | ||
| use datafusion_pruning::{FilePruner, PruningPredicate, build_pruning_predicate}; | ||
| use datafusion_pruning::{ | ||
| FilePruner, PruningPredicate, build_pruning_predicate_with_max_in_list_size, | ||
| }; | ||
|
|
||
| #[cfg(feature = "parquet_encryption")] | ||
| use datafusion_common::config::EncryptionFactoryOptions; | ||
|
|
@@ -289,6 +291,11 @@ pub(super) struct ParquetMorselizer { | |
| /// Maximum size of the predicate cache, in bytes. If none, uses | ||
| /// the arrow-rs default. | ||
| pub max_predicate_cache_size: Option<usize>, | ||
| /// Maximum `IN (...)` list size that the pruning predicate will rewrite | ||
| /// into per-value statistics checks. Lists longer than this skip | ||
| /// container-level pruning. Sourced from | ||
| /// `datafusion.execution.parquet.pruning_max_in_list_size`. | ||
| pub pruning_max_in_list_size: usize, | ||
| /// Whether to read row groups in reverse order | ||
| pub reverse_row_groups: bool, | ||
| /// Optional sort order used to reorder row groups by their min/max statistics. | ||
|
|
@@ -451,6 +458,7 @@ struct PreparedParquetOpen { | |
| expr_adapter_factory: Arc<dyn PhysicalExprAdapterFactory>, | ||
| predicate_creation_errors: Count, | ||
| max_predicate_cache_size: Option<usize>, | ||
| pruning_max_in_list_size: usize, | ||
| reverse_row_groups: bool, | ||
| sort_order_for_reorder: Option<LexOrdering>, | ||
| preserve_order: bool, | ||
|
|
@@ -850,6 +858,7 @@ impl ParquetMorselizer { | |
| expr_adapter_factory: Arc::clone(&self.expr_adapter_factory), | ||
| predicate_creation_errors, | ||
| max_predicate_cache_size: self.max_predicate_cache_size, | ||
| pruning_max_in_list_size: self.pruning_max_in_list_size, | ||
| reverse_row_groups: self.reverse_row_groups, | ||
| sort_order_for_reorder: self.sort_order_for_reorder.clone(), | ||
| preserve_order: self.preserve_order, | ||
|
|
@@ -1052,6 +1061,7 @@ impl MetadataLoadedParquetOpen { | |
| prepared.predicate.as_ref(), | ||
| &physical_file_schema, | ||
| &prepared.predicate_creation_errors, | ||
| prepared.pruning_max_in_list_size, | ||
| ); | ||
|
|
||
| // Only build page pruning predicate if page index is enabled | ||
|
|
@@ -1468,6 +1478,7 @@ impl RowGroupsPrunedParquetOpen { | |
| Arc::clone(reader_metadata.metadata()), | ||
| prepared.predicate_creation_errors.clone(), | ||
| prepared.file_metrics.predicate_evaluation_errors.clone(), | ||
| prepared.pruning_max_in_list_size, | ||
| )) | ||
| } | ||
| _ => None, | ||
|
|
@@ -1632,12 +1643,14 @@ pub(crate) fn build_pruning_predicates( | |
| predicate: Option<&Arc<dyn PhysicalExpr>>, | ||
| file_schema: &SchemaRef, | ||
| predicate_creation_errors: &Count, | ||
| max_in_list_size: usize, | ||
| ) -> Option<Arc<PruningPredicate>> { | ||
| let predicate = predicate.as_ref()?; | ||
| build_pruning_predicate( | ||
| build_pruning_predicate_with_max_in_list_size( | ||
| Arc::clone(predicate), | ||
| file_schema, | ||
| predicate_creation_errors, | ||
| max_in_list_size, | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -1720,6 +1733,7 @@ mod test { | |
| DefaultPhysicalExprAdapterFactory, replace_columns_with_literals, | ||
| }; | ||
| use datafusion_physical_plan::metrics::ExecutionPlanMetricsSet; | ||
| use datafusion_pruning::MAX_LIST_VALUE_SIZE_REWRITE; | ||
| use futures::StreamExt; | ||
| use futures::stream::BoxStream; | ||
| use object_store::{ObjectStore, ObjectStoreExt, memory::InMemory, path::Path}; | ||
|
|
@@ -1752,6 +1766,7 @@ mod test { | |
| enable_row_group_stats_pruning: bool, | ||
| coerce_int96: Option<TimeUnit>, | ||
| max_predicate_cache_size: Option<usize>, | ||
| pruning_max_in_list_size: usize, | ||
| reverse_row_groups: bool, | ||
| preserve_order: bool, | ||
| } | ||
|
|
@@ -1860,6 +1875,7 @@ mod test { | |
| enable_row_group_stats_pruning: false, | ||
| coerce_int96: None, | ||
| max_predicate_cache_size: None, | ||
| pruning_max_in_list_size: MAX_LIST_VALUE_SIZE_REWRITE, | ||
|
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. it is strange to me that these names are not the same -- I would expect something like pruning_max_in_list_size: PRUNING_MAX_IN_LIST_SIZE, |
||
| reverse_row_groups: false, | ||
| preserve_order: false, | ||
| } | ||
|
|
@@ -2037,6 +2053,7 @@ mod test { | |
| #[cfg(feature = "parquet_encryption")] | ||
| encryption_factory: None, | ||
| max_predicate_cache_size: self.max_predicate_cache_size, | ||
| pruning_max_in_list_size: self.pruning_max_in_list_size, | ||
| reverse_row_groups: self.reverse_row_groups, | ||
| sort_order_for_reorder: None, | ||
| virtual_state, | ||
|
|
||
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.
I think we could rewrite this to focus more on the end user visible effects to make it clearer what was going on