alamb commented on code in PR #25865:
URL: https://github.com/apache/datafusion/pull/25865#discussion_r4232172167
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -1596,33 +1587,27 @@ fn with_filters(predicates: Vec<Expr>, plan:
LogicalPlan) -> LogicalPlan {
}
}
-/// Does `expr` read nothing beyond the given window partition keys?
+/// Can `expr` be evaluated below a window with these `PARTITION BY` 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.
+/// A predicate that reads only the partition keys is constant within each
+/// partition, so filtering before the window drops whole partitions and leaves
+/// the surviving rows' window values unchanged. "Reads only the keys" is
checked
+/// structurally: every column reference must sit inside a subtree that is
equal
+/// to one of the keys. Given `PARTITION BY a, b + c`:
///
-/// 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 < 5`, `b + c = 4` and `(b + c) + 1 > 10` can be pushed down
Review Comment:
👍
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -1956,10 +2006,12 @@ mod tests {
)
}
- /// verifies that filters on partition expressions are not pushed, as the
single expression
- /// column is not available to the user, unlike with aggregations
+ /// verifies that a filter on an expression partition key is pushed; the
+ /// remaining shapes (mixed keys, operand order, subqueries, volatile
+ /// predicates, several windows) are covered in
+ /// `push_down_filter_regression.slt`
#[test]
- fn filter_expression_keep_window() -> Result<()> {
+ fn filter_expression_move_window() -> Result<()> {
Review Comment:
the filter is moved!
--
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]
