kosiew commented on code in PR #24817:
URL: https://github.com/apache/datafusion/pull/24817#discussion_r3948330464
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -778,10 +779,10 @@ struct AggrDynFilter {
/// The current bounds for the dynamic filter, updates during the
execution to
/// tighten the bound for more effective pruning.
///
- /// Each vector element is for the accumulators that support dynamic
filter.
- /// e.g. This `AggregateExec` has accumulator:
- /// min(a), avg(a), max(b)
- /// And this field stores [PerAccumulatorDynFilter(min(a)),
PerAccumulatorDynFilter(min(b))]
+ /// Each vector element corresponds to one aggregate expression. Dynamic
filtering
+ /// is enabled only when every aggregate expression is supported, so this
vector
+ /// contains an entry for every accumulator. For example, `min(a), max(b)`
produces
+ /// entries for `min(a)` and `max(b)`.
supported_accumulators_info: Vec<PerAccumulatorDynFilter>,
Review Comment:
Small naming suggestion, not blocking: would it make sense to rename
`supported_accumulators_info` to something like `accumulator_dyn_filter_info`?
With the new all-or-nothing behavior, this contains metadata for every
aggregate whenever dynamic filtering is enabled, so dropping `supported` might
make that invariant a little clearer.
##########
datafusion/sqllogictest/test_files/push_down_filter_regression.slt:
##########
@@ -415,15 +413,15 @@ CREATE EXTERNAL TABLE agg_dyn_mixed (a INT, b INT, c INT)
STORED AS PARQUET
LOCATION 'test_files/scratch/push_down_filter_regression/agg_dyn_mixed/';
-# -> DynamicFilter [ a < 1 OR a > 8 OR b > 12 ] (MIN(c+1) dropped as
unsupported)
+# No dynamic filter because not every aggregate has a safe predicate.
query TT
EXPLAIN ANALYZE SELECT MIN(a), MAX(a), MAX(b), MIN(c + 1) FROM agg_dyn_mixed;
Review Comment:
Could we add an execution-result assertion here as well? The current
`EXPLAIN ANALYZE` assertion confirms that the dynamic filter is absent, but it
does not directly verify the correctness issue this change is intended to fix:
the unsupported aggregate still needs to see every relevant row.
For this fixture, the expected result for `MIN(a), MAX(a), MAX(b), MIN(c +
1)` is `1, 8, 12, 71`.
It would also be good to make the fixture or scan order deterministic enough
that the old behavior actually prunes the row needed by `MIN(c + 1)`. With the
current two-file layout, that can depend on which file publishes its bounds
first. Ideally, this regression test should fail with the old implementation
because it produces the wrong result, rather than only because the plan
contains a dynamic filter.
--
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]
