-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(parquet): Enable Parquet filter_pushdown by default, with heurstic fallback when projecting few non-filter columns
#23420
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
Open
zhuqi-lucas
wants to merge
17
commits into
apache:main
Choose a base branch
from
zhuqi-lucas:qizhu/parquet-pushdown-adaptive-gate
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
2ce4952
feat(parquet): decline pushdown when projection has few non-filter co…
zhuqi-lucas 278ecff
Address alamb review: add config opt-out, upgrade guide, plan-display…
zhuqi-lucas 2620e86
fmt: cargo fmt after merging upstream/main
zhuqi-lucas db64af0
Restrict gate to file-schema columns (Copilot review)
zhuqi-lucas 64d8492
Fix CI test compile: add pushdown_filter_narrow_projection_gate to tw…
zhuqi-lucas 4e05b7d
Fix CI test failures: opt tests out of narrow-projection gate
zhuqi-lucas 6a73441
Defensive: skip gate when dynamic filter present + preserve source pu…
zhuqi-lucas 00101dd
test: disable narrow-projection gate in virtual-column rejection test
zhuqi-lucas 5ec0a50
slt: disable narrow-projection gate in pushdown-focused test files
zhuqi-lucas 73e6e5b
slt: disable narrow-projection gate in 3 more pushdown-focused test f…
zhuqi-lucas 32f09b7
test(sqllogictest): opt out of narrow-projection gate in dynamic_filt…
zhuqi-lucas c0fab95
Merge remote-tracking branch 'apache/main' into qizhu/parquet-pushdow…
alamb 5f5cff0
REname options
alamb 4721666
reorder for consistency
alamb 5dee386
reorder
alamb 5ea8476
Merge remote-tracking branch 'apache/main' into qizhu/parquet-pushdow…
alamb f4f8666
clean
alamb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ use arrow_schema::{DataType, Field}; | |
| use datafusion_common::config::ConfigOptions; | ||
| #[cfg(feature = "parquet_encryption")] | ||
| use datafusion_common::config::EncryptionFactoryOptions; | ||
| use datafusion_common::config::ParquetPushdownFilterMode; | ||
| use datafusion_datasource::as_file_source; | ||
| use datafusion_datasource::file_stream::FileOpener; | ||
| use datafusion_datasource::morsel::Morselizer; | ||
|
|
@@ -46,6 +47,7 @@ use datafusion_datasource::file_scan_config::FileScanConfig; | |
| use datafusion_functions::core::file_row_index::FileRowIndexFunc; | ||
| use datafusion_physical_expr::expressions::{Column, DynamicFilterTracking}; | ||
| use datafusion_physical_expr::projection::ProjectionExprs; | ||
| use datafusion_physical_expr::utils::collect_columns; | ||
| use datafusion_physical_expr::{EquivalenceProperties, conjunction}; | ||
| use datafusion_physical_expr_adapter::DefaultPhysicalExprAdapterFactory; | ||
| use datafusion_physical_expr_adapter::rewrite::{ | ||
|
|
@@ -827,7 +829,79 @@ impl FileSource for ParquetSource { | |
| // because even if scan pushdown is disabled we can still use the filters for stats pruning. | ||
| let config_pushdown_enabled = config.execution.parquet.pushdown_filters; | ||
| let table_pushdown_enabled = self.pushdown_filters(); | ||
| let pushdown_filters = table_pushdown_enabled || config_pushdown_enabled; | ||
| let mut pushdown_filters = table_pushdown_enabled || config_pushdown_enabled; | ||
| // Narrow-projection gate (issue #3463, discussion on PR #23369): | ||
|
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. we can probably reduce this comment substantially |
||
| // RowFilter has a fixed per-row machinery overhead that only pays | ||
| // for itself when the wide-column decode it lets us skip is | ||
| // meaningful. When the projection is narrow and the filter columns | ||
| // already cover most of it (typical for `GROUP BY col`-style | ||
| // queries such as ClickBench Q10/Q11/Q40), the overhead dominates | ||
| // and pushdown regresses. Decline pushdown for those scans; keep | ||
| // it for wider projections where the fast-path pays. | ||
| // | ||
| // Kept intentionally simple: a plan-time column-count check, | ||
| // used when `pushdown_filter_mode` is `auto` or `heuristic`. A | ||
| // future adaptive-placement pass ([#22883]) can supersede this | ||
| // with a runtime cost model behind the same `auto` mode. | ||
| // | ||
| // When declined: the filter stays above the scan in a | ||
| // `FilterExec` (correctness preserved) and the predicate is | ||
| // still injected into `ParquetSource` for stats / bloom / | ||
| // page-index pruning. | ||
| // | ||
| // Users who want the pre-existing "always push" behavior can | ||
| // set `pushdown_filter_mode = always`. | ||
| // | ||
| // [#22883]: https://github.com/apache/datafusion/issues/22883 | ||
| const PUSHDOWN_MIN_NON_FILTER_COLS: usize = 3; | ||
| let mut narrow_projection_gate_declined = false; | ||
| // Never gate a scan whose predicate already contains a dynamic | ||
| // filter — either in the incoming `filters` parameter, or already | ||
| // installed on `self.predicate` by an earlier optimizer rule (this | ||
| // is how `TopK`'s heap-threshold expression arrives: the sort | ||
| // pushdown / TopK rule injects the `DynamicFilterPhysicalExpr` | ||
| // into the scan's predicate before filter pushdown runs). Dynamic | ||
| // filters rely on the scan-time RowFilter/RowGroupPruner cascade | ||
| // to prune data as the threshold tightens, so declining pushdown | ||
| // here would silently disable the entire dynamic-RG-prune path | ||
| // for narrow `ORDER BY ... LIMIT` queries. | ||
| let existing_predicate_has_dynamic = self.predicate.as_ref().is_some_and(|p| { | ||
| DynamicFilterTracking::classify(p).contains_dynamic_filter() | ||
| }); | ||
| let incoming_filters_have_dynamic = filters | ||
| .iter() | ||
| .any(|f| DynamicFilterTracking::classify(f).contains_dynamic_filter()); | ||
| let has_dynamic_filter = | ||
| existing_predicate_has_dynamic || incoming_filters_have_dynamic; | ||
| if pushdown_filters | ||
| && !has_dynamic_filter | ||
| && config.execution.parquet.pushdown_filter_mode | ||
| != ParquetPushdownFilterMode::Always | ||
| { | ||
| // `TableSchema` layout is `[file, partition, virtual]`; only | ||
| // file columns are actually decoded from parquet, so partition | ||
| // and virtual columns don't count toward the "wide-decode | ||
| // saving" the RowFilter fast-path buys us. | ||
| let file_col_count = self.table_schema.file_schema().fields().len(); | ||
| let filter_col_indices: std::collections::HashSet<usize> = filters | ||
| .iter() | ||
| .flat_map(|f| collect_columns(f).into_iter().map(|c| c.index())) | ||
| .filter(|idx| *idx < file_col_count) | ||
| .collect(); | ||
| let non_filter_projected = self | ||
| .projection | ||
| .as_ref() | ||
| .iter() | ||
| .flat_map(|pe| collect_columns(&pe.expr)) | ||
| .map(|c| c.index()) | ||
| .filter(|idx| *idx < file_col_count && !filter_col_indices.contains(idx)) | ||
| .collect::<std::collections::HashSet<_>>() | ||
| .len(); | ||
| if non_filter_projected < PUSHDOWN_MIN_NON_FILTER_COLS { | ||
| pushdown_filters = false; | ||
| narrow_projection_gate_declined = true; | ||
| } | ||
| } | ||
|
|
||
| let mut source = self.clone(); | ||
| let filters: Vec<PushedDownPredicate> = filters | ||
|
|
@@ -864,7 +938,18 @@ impl FileSource for ParquetSource { | |
| None => conjunction(allowed_filters), | ||
| }; | ||
| source.predicate = Some(predicate); | ||
| source = source.with_pushdown_filters(pushdown_filters); | ||
| // Only persist the pushdown_filters bit on the source when this is | ||
| // a "real" config-driven decision. When the gate declined, we | ||
| // deliberately do NOT flip the source's pushdown flag to false — | ||
| // a later `try_pushdown_filters` call (e.g. from TopK's dynamic | ||
| // filter injection) needs the source to still be pushdown-eligible | ||
| // so that dynamic filter can install its RowFilter and drive the | ||
| // runtime RG-prune cascade. The current call still returns | ||
| // `PushedDown::No` for its incoming filters, so the FilterExec | ||
| // above the scan stays and correctness is preserved. | ||
| if !narrow_projection_gate_declined { | ||
| source = source.with_pushdown_filters(pushdown_filters); | ||
| } | ||
| let source = Arc::new(source); | ||
| // If pushdown_filters is false we tell our parents that they still have to handle the filters, | ||
| // even if we updated the predicate to include the filters (they will only be used for stats pruning). | ||
|
|
@@ -1867,7 +1952,13 @@ mod tests { | |
| ) | ||
| .expect("file_row_index should rewrite to the row_number virtual column"); | ||
|
|
||
| let config = ConfigOptions::default(); | ||
| let mut config = ConfigOptions::default(); | ||
| // Force unconditional pushdown so this test can exercise the | ||
| // virtual-column rejection path independently. The scan projection | ||
| // here has 0 non-filter file columns, which would otherwise trip | ||
| // the narrow-projection heuristic and mark every filter as | ||
| // `PushedDown::No` regardless of virtual-column content. | ||
| config.execution.parquet.pushdown_filter_mode = ParquetPushdownFilterMode::Always; | ||
| let prop = source | ||
| .try_pushdown_filters(vec![pushable, virtual_only, mixed, row_index], &config) | ||
| .expect("try_pushdown_filters must not error"); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is my proposed configuration API -- an enum to control the filter pushdown behavior (and we will add fancier ones here like
auto😎 )