zhuqi-lucas commented on code in PR #25865:
URL: https://github.com/apache/datafusion/pull/25865#discussion_r4227471790
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -1172,21 +1177,27 @@ impl OptimizerRule for PushDownFilter {
let mut keep_predicates = vec![];
let mut push_predicates = vec![];
for expr in predicates {
- let cols = expr.column_refs();
- if cols.iter().all(|c|
potential_partition_keys.contains(c)) {
+ // A volatile predicate has to stay above the window.
Pushing it
+ // changes which rows the window function sees, and so the
value
+ // it computes for the rows that do survive. Checking this
first
Review Comment:
Trimmed to one sentence.
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -1585,6 +1596,63 @@ fn with_filters(predicates: Vec<Expr>, plan:
LogicalPlan) -> LogicalPlan {
}
}
+/// Does `expr` read nothing beyond the given window partition keys?
+///
+/// A subtree that is exactly one of the keys counts as read in full, so a
Review Comment:
Rewrote it around your example: PARTITION BY a, b + c; pushed a < 5, b + c =
4, (b + c) + 1 > 10; not pushed d < 5, b < 5, c + b = 4.
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -1585,6 +1596,63 @@ fn with_filters(predicates: Vec<Expr>, plan:
LogicalPlan) -> LogicalPlan {
}
}
+/// Does `expr` read nothing beyond the given window partition keys?
+///
+/// A subtree that is exactly one of the keys counts as read in full, so a
Review Comment:
Done, the doc now carries exactly that example.
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -1585,6 +1596,63 @@ fn with_filters(predicates: Vec<Expr>, plan:
LogicalPlan) -> LogicalPlan {
}
}
+/// Does `expr` read nothing beyond the given window partition keys?
+///
+/// A subtree that is exactly one of the keys counts as read in full, so a
+/// predicate on an *expression* key, say `NULLIF(c, '') IS NOT NULL` against
+/// `PARTITION BY NULLIF(c, '')`, qualifies even though the column it
ultimately
+/// reads (`c`) is not a key on its own. Such a predicate is constant within
each
+/// partition, so applying it below the window drops whole partitions and
leaves
+/// every surviving row's window value unchanged.
+///
+/// Matching is structural, which makes this conservative rather than wrong: a
+/// predicate written `b + a` does not match a key written `a + b`, and is
simply
+/// left above the window.
+///
+/// A node carrying a subquery counts as reading something else, whatever the
Review Comment:
Cut to one sentence.
##########
datafusion/sqllogictest/test_files/push_down_filter_regression.slt:
##########
@@ -723,3 +723,92 @@ query I
SELECT sum(c) FROM (SELECT random() < 0.5 AS k, count(*) AS c FROM
generate_series(1, 10000) GROUP BY random() < 0.5) WHERE k OR NOT k;
----
10000
+
+# Window filter pushdown over an expression PARTITION BY key.
+#
+# A predicate that reads only the partition keys is constant within a
partition,
+# so pushing it below the window drops whole partitions and leaves every
+# surviving row's window value alone. The unit tests pin where the filter
lands;
Review Comment:
Trimmed.
##########
datafusion/sqllogictest/test_files/push_down_filter_regression.slt:
##########
@@ -723,3 +723,92 @@ query I
SELECT sum(c) FROM (SELECT random() < 0.5 AS k, count(*) AS c FROM
generate_series(1, 10000) GROUP BY random() < 0.5) WHERE k OR NOT k;
----
10000
+
+# Window filter pushdown over an expression PARTITION BY key.
+#
+# A predicate that reads only the partition keys is constant within a
partition,
+# so pushing it below the window drops whole partitions and leaves every
+# surviving row's window value alone. The unit tests pin where the filter
lands;
+# these pin that the answers do not move, which is what a wrong push breaks.
+
+statement ok
+create table window_expr_key(k varchar, v int) as values
+ ('a', 1), ('a', 2), ('a', 3),
+ ('', 4), ('', 5),
+ ('b', 6);
+
+# Partitions under NULLIF(k, '') are 'a' => {1,2,3}, NULL => {4,5}, 'b' => {6}.
Review Comment:
Added EXPLAIN to every case, with logical_plan_only.
--
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]
