Add support for aggregate expressions with filters #585

andygrove · 2022-08-19T12:04:40Z

Closes #577

coveralls · 2022-08-19T12:07:31Z

Pull Request Test Coverage Report for Build 2980739859

31 of 34 (91.18%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.01%) to 85.558%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/dialect/hive.rs	1	2	50.0%
src/dialect/mod.rs	1	2	50.0%
src/dialect/postgresql.rs	1	2	50.0%

Files with Coverage Reduction	New Missed Lines	%
src/ast/mod.rs	1	77.79%

Totals
Change from base Build 2957846519:	0.01%
Covered Lines:	9728
Relevant Lines:	11370

💛 - Coveralls

andygrove · 2022-08-19T12:23:15Z

Code needs cleaning up but maybe this approach can work?

andygrove · 2022-08-19T13:23:06Z

tests/sqlparser_hive.rs

+
+#[test]
+fn filter_as_alias() {
+    let rename = "SELECT name filter FROM region";


we need a different style test - this fails because it is re-written as SELECT name AS filter FROM region

xrl · 2022-08-19T16:00:15Z

This was the approach I was crawling towards. Hopefully I'm not being too obvious, but the filtering_during_aggregation test case fails:

#[test]
fn filtering_during_aggregation() {
    let rename = "SELECT array_agg(name) FILTER (WHERE name IS NOT NULL) FROM region";
    println!("{}", hive().verified_stmt(rename));
}

fails with

called `Result::unwrap()` on an `Err` value: ParserError("Expected end of statement, found: name")
thread 'filtering_during_aggregation' panicked at 'called `Result::unwrap()` on an `Err` value: ParserError("Expected end of statement, found: name")', src/test_utils.rs:86:61
stack backtrace:

andygrove · 2022-08-19T20:35:56Z

This was the approach I was crawling towards. Hopefully I'm not being too obvious, but the filtering_during_aggregation test case fails:

Ugh, this was passing at one point. I will try and take another look over the weekend.

xrl · 2022-08-22T18:20:43Z

I think this is failing because the get_next_precedence function doesn't consider the FILTER token.

[2022-08-22T18:16:08Z DEBUG sqlparser::parser] Parsing sql 'SELECT array_agg(name) FILTER (WHERE name IS NOT NULL) FROM region'...
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] parse query
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] parsing expr
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] parsing expr
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] prefix: Identifier(Ident { value: "name", quote_style: None })
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] get_next_precedence() RParen
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] 0: ) 1: FILTER 2: (
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] next precedence: 0
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] breaking!
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] prefix: Function(Function { name: ObjectName([Ident { value: "array_agg", quote_style: None }]), args: [Unnamed(Expr(Identifier(Ident { value: "name", quote_style: None })))], over: None, distinct: false, special: false })
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] get_next_precedence() Word(Word { value: "FILTER", quote_style: None, keyword: FILTER })
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] 0: FILTER 1: ( 2: WHERE
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] next precedence: 0
[2022-08-22T18:16:08Z DEBUG sqlparser::parser] breaking!

xrl · 2022-08-22T18:25:51Z

Hmm, I think the FILTER is applied to the specific expression in the select. It's not for the whole select, but for each column. The example of this SQL that led me down this rabbit hole:

   SELECT
     "organization_id"
   , "provider_pg_id" "provider_id"
   , "array_agg"("status") FILTER (WHERE ("badge_type" = 'AW'))[1] "aw_badge"
   , "array_agg"("status") FILTER (WHERE ("badge_type" = 'HBS'))[1] "hbs_badge"
   , "array_agg"("status") FILTER (WHERE ("badge_type" = 'RWE'))[1] "rwe_badge"
[[[ SNIP ]]]

so I think the logic has to be pushed "deeper" in to the parser logic, pushed from parse_select down to parse_select_item

jdye64 · 2022-09-01T17:13:57Z

src/dialect/hive.rs

@@ -36,4 +36,8 @@ impl Dialect for HiveDialect {
            || ch == '{'
            || ch == '}'
    }
+
+    fn supports_filter_during_aggregation(&self) -> bool {


I believe we would want to add this to src/dialect/postgres.rs as well since postgresql supports FILTER also https://www.postgresql.org/docs/current/sql-expressions.html

andygrove · 2022-09-01T21:50:19Z

I will resume work on this tomorrow

andygrove · 2022-09-02T17:16:47Z

@xrl @jdye64 @alamb This is now ready for review

xrl · 2022-09-02T20:55:13Z

This work got me closer but I'm unable to parse expressions with the filter proceeded a [1] term.

    #[test]
    fn filter_with_array_access_sql() {
        env_logger::init();

        let sql = r#"SELECT
     "organization_id"
   , "provider_pg_id" "provider_id"
   , "array_agg"("status") FILTER (WHERE ("badge_type" = 'AW'))[1] "aw_badge"
   FROM
   asdf"#;
        sqlparser::parser::Parser::parse_sql(&sqlparser::dialect::HiveDialect {}, sql).unwrap();
    }

using this PR as a git crate dependency now yields:

     Running unittests src/lib.rs (target/debug/deps/sparkle_view_analyzer-f90def7b85a0f85e)
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] Parsing sql 'SELECT
         "organization_id"
       , "provider_pg_id" "provider_id"
       , "array_agg"("status") FILTER (WHERE ("badge_type" = 'AW'))[1] "aw_badge"
       FROM
       asdf'...
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] parsing expr
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] prefix: Identifier(Ident { value: "organization_id", quote_style: Some('"') })
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] get_next_precedence() Comma
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] 0: , 1: "provider_pg_id" 2: "provider_id"
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] next precedence: 0
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] parsing expr
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] prefix: Identifier(Ident { value: "provider_pg_id", quote_style: Some('"') })
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] get_next_precedence() Word(Word { value: "provider_id", quote_style: Some('"'), keyword: NoKeyword })
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] 0: "provider_id" 1: , 2: "array_agg"
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] next precedence: 0
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] parsing expr
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] parsing expr
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] prefix: Identifier(Ident { value: "status", quote_style: Some('"') })
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] get_next_precedence() RParen
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] 0: ) 1: FILTER 2: (
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] next precedence: 0
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] prefix: Function(Function { name: ObjectName([Ident { value: "array_agg", quote_style: Some('"') }]), args: [Unnamed(Expr(Identifier(Ident { value: "status", quote_style: Some('"') })))], over: None, distinct: false, special: false })
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] get_next_precedence() Word(Word { value: "FILTER", quote_style: None, keyword: FILTER })
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] 0: FILTER 1: ( 2: WHERE
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] next precedence: 0
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] parsing expr
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] parsing expr
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] prefix: Identifier(Ident { value: "badge_type", quote_style: Some('"') })
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] get_next_precedence() Eq
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] 0: = 1: 'AW' 2: )
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] next precedence: 20
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] parsing expr
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] prefix: Value(SingleQuotedString("AW"))
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] get_next_precedence() RParen
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] 0: ) 1: ) 2: [
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] next precedence: 0
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] get_next_precedence() RParen
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] 0: ) 1: ) 2: [
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] next precedence: 0
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] prefix: Nested(BinaryOp { left: Identifier(Ident { value: "badge_type", quote_style: Some('"') }), op: Eq, right: Value(SingleQuotedString("AW")) })
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] get_next_precedence() RParen
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] 0: ) 1: [ 2: 1
[2022-09-02T20:54:48Z DEBUG sqlparser::parser] next precedence: 0

called `Result::unwrap()` on an `Err` value: ParserError("Expected end of statement, found: [")
thread 'graph::tests::filter_with_array_access_sql' panicked at 'called `Result::unwrap()` on an `Err` value: ParserError("Expected end of statement, found: [")', sparkle-view-analyzer/src/graph/mod.rs:336:88

background: https://trino.io/docs/current/functions/aggregate.html

array_agg(x) → array<[same as input]>

    Returns an array created from the input x elements.

xrl · 2022-09-03T04:58:18Z

Should the select item ("some_name")[1][20] be parseable? Right now it's not surviving a round trip.

jdye64

LGTM

andygrove · 2022-09-06T16:02:55Z

Should the select item ("some_name")[1][20] be parseable? Right now it's not surviving a round trip.

Hi @xrl. I am not sure I understand this. Is the goal to extract one array element from the aggregated array? I tried this in postgres and it also did not like this syntax:

postgres=# select array_agg(foo)[1] from arrays;
ERROR:  syntax error at or near "["
LINE 1: select array_agg(foo)[1] from arrays;
                             ^
postgres=# select array_agg(foo) filter (where foo is not null)[1] from arrays;
ERROR:  syntax error at or near "["
LINE 1: ...lect array_agg(foo) filter (where foo is not null)[1] from a...

Is this query working for you in Hive/Trino?

Maybe you could do this with a subquery for the aggregation? This worked for me in Postgres:

select x[1] from (select array_agg(foo) as x from arrays) y;

andygrove · 2022-09-06T16:03:37Z

@alamb Could you review when you have time

xrl · 2022-09-06T16:10:04Z

Is this query working for you in Hive/Trino?

Yes, array agg then pull out an element. And the query does work on AWS Athena (presto under the hood).

andygrove · 2022-09-06T16:15:54Z

Is this query working for you in Hive/Trino?

Yes, array agg then pull out an element. And the query does work on AWS Athena (presto under the hood).

Interesting. Ok, I filed a new issue for supporting this. I think this is separate from the FILTER (WHERE) support.

alamb

Every time I look at a PR in this repo I feel like I learn something new about SQL. TIL FILTER (where) 🤯

Thanks @andygrove this PR looks reasonable to me

alamb · 2022-09-08T20:51:38Z

src/dialect/postgresql.rs

@@ -42,6 +42,10 @@ impl Dialect for PostgreSqlDialect {
            None
        }
    }
+
+    fn supports_filter_during_aggregation(&self) -> bool {


For what it is worth, I think the DataFusion actually uses the Generic Dialect, https://github.com/apache/arrow-datafusion/blob/0084aeb686b318cbdb49cab00cb8f15c9f520d1e/datafusion/sql/src/parser.rs#L102

Thus we may want to add return true from supports_filter_during_aggregation in https://github.com/sqlparser-rs/sqlparser-rs/blob/main/src/dialect/generic.rs as well

Thanks. Dask SQL has their own dialect where they want to use this, but it makes sense to add to the dialect that DataFusion is using as well.

andygrove added 2 commits August 19, 2022 06:00

Add support for filter during aggregation .. FILTER (WHERE ..)

0a777b2

make this dialect specific

e2a8432

andygrove mentioned this pull request Aug 19, 2022

Hive dialect cannot parse WHERE nested in FILTER #577

Closed

andygrove marked this pull request as draft August 19, 2022 12:14

andygrove changed the title ~~Hive dialect cannot parse WHERE nested in FILTER~~ WIP: Hive dialect cannot parse WHERE nested in FILTER Aug 19, 2022

fix

6101026

andygrove commented Aug 19, 2022

View reviewed changes

jdye64 suggested changes Sep 1, 2022

View reviewed changes

andygrove added 3 commits September 2, 2022 10:58

save progress

29276e2

Merge remote-tracking branch 'upstream/main' into hive-filter

d663a3e

Merge remote-tracking branch 'upstream/main' into hive-filter

93ac2f3

andygrove changed the title ~~WIP: Hive dialect cannot parse WHERE nested in FILTER~~ Add support for aggregate expressions with filters Sep 2, 2022

andygrove added 2 commits September 2, 2022 11:11

re-implement

9cd5782

fix

96ff349

andygrove marked this pull request as ready for review September 2, 2022 17:15

revert debug

315916b

andygrove mentioned this pull request Sep 2, 2022

[DF] Remaining DataFusion / sqlparser work for the GA release dask-contrib/dask-sql#706

Closed

24 tasks

jdye64 approved these changes Sep 3, 2022

View reviewed changes

andygrove mentioned this pull request Sep 6, 2022

Add support for array_agg("name")[1] #606

Open

andygrove requested review from alamb, Dandandan and houqp September 7, 2022 14:11

andygrove merged commit 303f80f into sqlparser-rs:main Sep 8, 2022

andygrove deleted the hive-filter branch September 8, 2022 19:08

alamb reviewed Sep 8, 2022

View reviewed changes

charlesbluca mentioned this pull request Sep 13, 2022

[DF] Add support for filtered groupby aggregations dask-contrib/dask-sql#760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for aggregate expressions with filters #585

Add support for aggregate expressions with filters #585

andygrove commented Aug 19, 2022

coveralls commented Aug 19, 2022 •

edited

andygrove commented Aug 19, 2022

andygrove Aug 19, 2022

xrl commented Aug 19, 2022

andygrove commented Aug 19, 2022

xrl commented Aug 22, 2022

xrl commented Aug 22, 2022

jdye64 Sep 1, 2022

andygrove Sep 2, 2022

andygrove commented Sep 1, 2022

andygrove commented Sep 2, 2022

xrl commented Sep 2, 2022 •

edited

xrl commented Sep 3, 2022

jdye64 left a comment

andygrove commented Sep 6, 2022

andygrove commented Sep 6, 2022

xrl commented Sep 6, 2022

andygrove commented Sep 6, 2022

alamb left a comment

alamb Sep 8, 2022

andygrove Sep 8, 2022

Add support for aggregate expressions with filters #585

Add support for aggregate expressions with filters #585

Conversation

andygrove commented Aug 19, 2022

coveralls commented Aug 19, 2022 • edited

Pull Request Test Coverage Report for Build 2980739859

💛 - Coveralls

andygrove commented Aug 19, 2022

andygrove Aug 19, 2022

Choose a reason for hiding this comment

xrl commented Aug 19, 2022

andygrove commented Aug 19, 2022

xrl commented Aug 22, 2022

xrl commented Aug 22, 2022

jdye64 Sep 1, 2022

Choose a reason for hiding this comment

andygrove Sep 2, 2022

Choose a reason for hiding this comment

andygrove commented Sep 1, 2022

andygrove commented Sep 2, 2022

xrl commented Sep 2, 2022 • edited

xrl commented Sep 3, 2022

jdye64 left a comment

Choose a reason for hiding this comment

andygrove commented Sep 6, 2022

andygrove commented Sep 6, 2022

xrl commented Sep 6, 2022

andygrove commented Sep 6, 2022

alamb left a comment

Choose a reason for hiding this comment

alamb Sep 8, 2022

Choose a reason for hiding this comment

andygrove Sep 8, 2022

Choose a reason for hiding this comment

coveralls commented Aug 19, 2022 •

edited

xrl commented Sep 2, 2022 •

edited