Add logical plan support for aggregate expressions with filters (and upgrade to sqlparser 0.23) #3405

andygrove · 2022-09-08T21:38:51Z

Which issue does this PR close?

Part of #2214

Rationale for this change

Adds support to the SQL query planner and logical plan for aggregate expressions with filters e.g. aggr_expr(foo) FILTER (WHERE ...)

What changes are included in this PR?

Expr::AggregateFunction and Expr::AggregateUDF have a new filter attribute
SQL parser updated to populate this
Physical plan and proto throw an error if this filter is populated

Are there any user-facing changes?

Yes, Expr API change

codecov-commenter · 2022-09-08T22:52:36Z

Codecov Report

Merging #3405 (00b7fdb) into master (c5c1dae) will increase coverage by 0.00%.
The diff coverage is 85.50%.

@@           Coverage Diff           @@
##           master    #3405   +/-   ##
=======================================
  Coverage   85.68%   85.68%           
=======================================
  Files         298      298           
  Lines       54645    54700   +55     
=======================================
+ Hits        46820    46868   +48     
- Misses       7825     7832    +7

Impacted Files	Coverage Δ
datafusion/expr/src/expr_fn.rs	`91.06% <ø> (ø)`
datafusion/expr/src/expr_rewriter.rs	`80.53% <0.00%> (-0.31%)`	⬇️
datafusion/expr/src/udaf.rs	`37.50% <ø> (ø)`
datafusion/core/src/physical_plan/planner.rs	`77.25% <50.00%> (-0.13%)`	⬇️
datafusion/sql/src/utils.rs	`46.51% <75.00%> (+0.20%)`	⬆️
datafusion/sql/src/planner.rs	`80.88% <77.27%> (-0.01%)`	⬇️
datafusion/expr/src/expr.rs	`83.29% <94.44%> (+0.34%)`	⬆️
datafusion/core/tests/sql/select.rs	`99.78% <100.00%> (+<0.01%)`	⬆️
...fusion/optimizer/src/single_distinct_to_groupby.rs	`98.34% <100.00%> (+<0.01%)`	⬆️
datafusion/proto/src/from_proto.rs	`39.86% <100.00%> (+0.25%)`	⬆️
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

andygrove · 2022-09-10T15:06:06Z

cc @ayushdg

alamb

Thanks @andygrove

Looks good to me -- I had some comments, but nothing that needs to be done in this PR (could be done in a follow on).

https://www.postgresql.org/docs/current/sql-expressions.html

4.2.7. Aggregate Expressions

Today I learned that postgres aggregates support all sorts of semi-wild things (like an ORDER BY clause)

alamb · 2022-09-12T11:10:48Z

datafusion/expr/src/expr.rs

@@ -231,6 +231,8 @@ pub enum Expr {
        args: Vec<Expr>,
        /// Whether this is a DISTINCT aggregation or not
        distinct: bool,
+        /// Optional filter


Suggested change

/// Optional filter

/// Optional filter applied prior to aggregating

datafusion/expr/src/expr.rs

alamb · 2022-09-12T11:16:43Z

datafusion/proto/src/lib.rs

@@ -1009,6 +1012,7 @@ mod roundtrip_tests {
        let test_expr = Expr::AggregateUDF {
            fun: Arc::new(dummy_agg.clone()),
            args: vec![lit(1.0_f64)],
+            filter: Some(Box::new(lit(true))),


alamb · 2022-09-12T11:18:11Z

datafusion/proto/src/to_proto.rs

+                            filter: match filter {
+                                Some(e) => Some(Box::new(e.as_ref().try_into()?)),
+                                None => None,
+                            }


I think you can write this kind of thing somewhat more concisely via

Suggested change

filter: match filter {

Some(e) => Some(Box::new(e.as_ref().try_into()?)),

None => None,

}

filter: filter

.map(|filter| Box::new(e.as_ref().try_into()))

.transpose()?

I experimented with this locally but could not get this to work.

alamb · 2022-09-12T11:18:44Z

datafusion/sql/src/planner.rs

+                    Expr::AggregateFunction {
+                        fun, args, distinct, ..
+                    } =>  Ok(Expr::AggregateFunction { fun, args, distinct, filter: Some(Box::new(self.sql_expr_to_logical_expr(*filter, schema, ctes)?)) }),
+                    _ => Err(DataFusionError::Internal("".to_string()))


The error message here appears to be missing

andygrove · 2022-09-12T16:54:34Z

Thanks for the review @alamb. I fixed the error message.

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

ursabot · 2022-09-12T18:23:31Z

Benchmark runs are scheduled for baseline = 1d4a928 and contender = 30e8cef. 30e8cef is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Use sqlparser-0.23

1a1957d

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner labels Sep 8, 2022

andygrove marked this pull request as draft September 8, 2022 21:39

Add filter to aggregate expressions

a4ea9af

github-actions bot added the optimizer Optimizer rules label Sep 8, 2022

andygrove changed the title ~~Use sqlparser 0.23~~ Add logical plan support for aggregate expressions with filters (and upgrade to sqlparser 0.23) Sep 8, 2022

andygrove added the api change Changes the API exposed to users of the crate label Sep 8, 2022

andygrove mentioned this pull request Sep 9, 2022

Support for Selective Aggregates, Filter clause #2214

Closed

andygrove added 2 commits September 10, 2022 08:37

clippy

d309a3c

implement protobuf serde

7380307

andygrove marked this pull request as ready for review September 10, 2022 14:58

andygrove requested a review from alamb September 10, 2022 15:05

alamb reviewed Sep 12, 2022

View reviewed changes

alamb approved these changes Sep 12, 2022

View reviewed changes

andygrove added 3 commits September 12, 2022 10:01

clippy

baf12a4

Merge remote-tracking branch 'apache/master' into sqlparser-0.23

9461974

fix error message

00b7fdb

Update datafusion/expr/src/expr.rs

b0d158a

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

andygrove merged commit 30e8cef into apache:master Sep 12, 2022

andygrove deleted the sqlparser-0.23 branch September 12, 2022 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logical plan support for aggregate expressions with filters (and upgrade to sqlparser 0.23) #3405

Add logical plan support for aggregate expressions with filters (and upgrade to sqlparser 0.23) #3405

andygrove commented Sep 8, 2022 •

edited

Loading

codecov-commenter commented Sep 8, 2022 •

edited

Loading

andygrove commented Sep 10, 2022

alamb left a comment

alamb Sep 12, 2022

alamb Sep 12, 2022

alamb Sep 12, 2022

andygrove Sep 12, 2022

alamb Sep 12, 2022

andygrove commented Sep 12, 2022

ursabot commented Sep 12, 2022

	/// Optional filter
	/// Optional filter applied prior to aggregating

Add logical plan support for aggregate expressions with filters (and upgrade to sqlparser 0.23) #3405

Add logical plan support for aggregate expressions with filters (and upgrade to sqlparser 0.23) #3405

Conversation

andygrove commented Sep 8, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Sep 8, 2022 • edited Loading

Codecov Report

andygrove commented Sep 10, 2022

alamb left a comment

Choose a reason for hiding this comment

alamb Sep 12, 2022

Choose a reason for hiding this comment

alamb Sep 12, 2022

Choose a reason for hiding this comment

alamb Sep 12, 2022

Choose a reason for hiding this comment

andygrove Sep 12, 2022

Choose a reason for hiding this comment

alamb Sep 12, 2022

Choose a reason for hiding this comment

andygrove commented Sep 12, 2022

ursabot commented Sep 12, 2022

andygrove commented Sep 8, 2022 •

edited

Loading

codecov-commenter commented Sep 8, 2022 •

edited

Loading