gabotechs commented on code in PR #17378:
URL: https://github.com/apache/datafusion/pull/17378#discussion_r2318690269
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2469,6 +2469,20 @@ impl Window {
window_func_dependencies.extend(new_deps);
}
+ // Validate that FILTER if present is only used with aggregate window
functions
Review Comment:
```suggestion
// Validate that FILTER is present only if used with aggregate
window functions
```
##########
datafusion/sqllogictest/test_files/window.slt:
##########
@@ -5898,3 +5898,115 @@ physical_plan
06)----------RepartitionExec: partitioning=Hash([k@1], 2), input_partitions=2
07)------------ProjectionExec: expr=[CAST(v@1 AS Int64) as __common_expr_1,
k@0 as k, time@2 as time]
08)--------------DataSourceExec: partitions=2, partition_sizes=[5, 4]
+
+
+# FILTER clause with window functions
Review Comment:
Giving here some ideas for other tests:
- A test that uses more than 1 expression in the filter
```sql
SELECT
c1,
c2,
SUM(c2) FILTER (WHERE c2 >= 2 AND c1 = 0 AND c2 < 4) OVER (ORDER BY c1, c2
ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) as sum1,
COUNT(c2) FILTER (WHERE c2 >= 2 AND c1 = 0 AND c2 < 4) OVER (ORDER BY c1, c2
ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) as count1,
ARRAY_AGG(c2) FILTER (WHERE c2 >= 2 AND c1 = 0 AND c2 < 4) OVER (ORDER BY
c1, c2 ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) as array_agg1,
FROM test
ORDER BY c1, c2
LIMIT 5
```
- A test that performs an invalid comparison inside the filter expression
and checks that the error is propagated correctly
```sql
SELECT
c1,
SUM(c2) FILTER (WHERE c2 >= []) OVER (ORDER BY c1, c2 ROWS BETWEEN UNBOUNDED
PRECEDING AND CURRENT ROW) as sum1,
FROM test
LIMIT 5
```
##########
datafusion/expr/src/expr.rs:
##########
@@ -2993,6 +3006,10 @@ impl Display for SchemaDisplay<'_> {
write!(f, " {null_treatment}")?;
}
+ if let Some(filter) = filter {
+ write!(f, " FILTER (WHERE {filter})")?;
+ }
Review Comment:
Maybe cover this in some test? maybe there some roundtrip tests somewhere
that go from SQL -> logical plan -> SQL
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -317,6 +317,7 @@ pub fn serialize_expr(
// TODO: support null treatment in proto
null_treatment: _,
distinct: _,
+ filter: _,
Review Comment:
It would be nice to support this at some point, projects bringing
distributed capabilities for DataFusion would find this very useful.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
