Copilot commented on code in PR #23420:
URL: https://github.com/apache/datafusion/pull/23420#discussion_r3551917826
##########
datafusion/datasource-parquet/src/source.rs:
##########
@@ -827,7 +828,43 @@ 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;
+
+ // Simple heuristic (issue #3463, discussion on PR #23369):
+ // 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 with
+ // no tuning knob. A future adaptive-placement pass (#22883) can
+ // supersede this with a runtime cost model.
+ //
+ // 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.
+ const PUSHDOWN_MIN_NON_FILTER_COLS: usize = 3;
+ if pushdown_filters {
+ let filter_col_indices: std::collections::HashSet<usize> = filters
+ .iter()
+ .flat_map(|f| collect_columns(f).into_iter().map(|c|
c.index()))
+ .collect();
+ let non_filter_projected = self
+ .projection
+ .as_ref()
+ .iter()
+ .flat_map(|pe| collect_columns(&pe.expr))
+ .map(|c| c.index())
+ .filter(|idx| !filter_col_indices.contains(idx))
+ .collect::<std::collections::HashSet<_>>()
+ .len();
Review Comment:
The heuristic counts "non-filter projected" columns across the full table
schema. However `ParquetSource::projection` includes partition and virtual
columns (`TableSchema` layout is `[file, partition, virtual]`), and those
columns don’t incur Parquet decode cost. Counting them can keep pushdown
enabled for partitioned tables even when the projected *file* columns are
narrow (the case this heuristic aims to protect), which can reintroduce the
regressions described in the PR rationale.
Consider restricting both the filter-column set and the projected-column
count to *file* columns only (indices `<
self.table_schema.file_schema().fields().len()`).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
