Parser error with GROUP BY with multiple filters on DataFusion 45

[mildbyte]

Describe the bug

Running this query:

SELECT
  c1,
  SUM(c2) FILTER (WHERE c2 >= 20) AS sum_c2,
  AVG(c3) FILTER (WHERE c3 <= 70) AS avg_c3
FROM test_table GROUP BY c1

causes a parser error:

SQL(ParserError("Expected end of statement, found: ("), None)

To Reproduce

Repro repo with a lockfile is available on https://github.com/mildbyte/datafusion-agg-filter-repro:

use datafusion::prelude::{SessionConfig, SessionContext};

#[tokio::main]
async fn main() {
    let sql = "SELECT
  c1,
  SUM(c2) FILTER (WHERE c2 >= 20) AS sum_c2,
  AVG(c3) FILTER (WHERE c3 <= 70) AS avg_c3
FROM test_table GROUP BY c1";
    let config = SessionConfig::new();
    let context = SessionContext::new_with_config(config);
    let stream = context.sql(sql).await.unwrap();
}

cargo run
...
warning: `datafusion-agg-filter-repro` (bin "datafusion-agg-filter-repro") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1m 01s
     Running `target/debug/datafusion-agg-filter-repro`

thread 'main' panicked at src/main.rs:12:41:
called `Result::unwrap()` on an `Err` value: SQL(ParserError("Expected end of statement, found: ("), None)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Expected behavior

Since I didn't create the tables, I don't expect the query to actually run, but I would expect it to get to the plan phase and error then instead of erroring in the parser.

This test case is already covered in DF: https://github.com/apache/datafusion/blame/980931c5a70b8b08c918195803145f64f1ec901f/datafusion/sqllogictest/test_files/aggregate.slt#L4770-L4778

Additional context

rustc 1.86.0-nightly (9a1d156f3 2025-01-19)
datafusion 45.0.0
sqlparser 0.53.0

This is the AST that's output by DF's parser (though I'm using 0.52.0 in this example). It looks like with the default dialect, it treats SUM(c2) FILTER as FILTER being an alias for SUM(c2), then it considers the query to be over:

Details

Statement(
    Query(
        Query {
            with: None,
            body: Select(
                Select {
                    select_token: TokenWithSpan {
                        token: Word(
                            Word {
                                value: "SELECT",
                                quote_style: None,
                                keyword: SELECT,
                            },
                        ),
                        span: Span(Location(0,0)..Location(0,0)),
                    },
                    distinct: None,
                    top: None,
                    top_before_distinct: false,
                    projection: [
                        UnnamedExpr(
                            Identifier(
                                Ident {
                                    value: "c1",
                                    quote_style: None,
                                    span: Span(Location(0,0)..Location(0,0)),
                                },
                            ),
                        ),
                        ExprWithAlias {
                            expr: Function(
                                Function {
                                    name: ObjectName(
                                        [
                                            Ident {
                                                value: "SUM",
                                                quote_style: None,
                                                span: Span(Location(0,0)..Location(0,0)),
                                            },
                                        ],
                                    ),
                                    uses_odbc_syntax: false,
                                    parameters: None,
                                    args: List(
                                        FunctionArgumentList {
                                            duplicate_treatment: None,
                                            args: [
                                                Unnamed(
                                                    Expr(
                                                        Identifier(
                                                            Ident {
                                                                value: "c2",
                                                                quote_style: None,
                                                                span: Span(Location(0,0)..Location(0,0)),
                                                            },
                                                        ),
                                                    ),
                                                ),
                                            ],
                                            clauses: [],
                                        },
                                    ),
                                    filter: None,
                                    null_treatment: None,
                                    over: None,
                                    within_group: [],
                                },
                            ),
                            alias: Ident {
                                value: "FILTER",
                                quote_style: None,
                                span: Span(Location(0,0)..Location(0,0)),
                            },
                        },
                    ],
                    into: None,
                    from: [],
                    lateral_views: [],
                    prewhere: None,
                    selection: None,
                    group_by: Expressions(
                        [],
                        [],
                    ),
                    cluster_by: [],
                    distribute_by: [],
                    sort_by: [],
                    having: None,
                    named_window: [],
                    qualify: None,
                    window_before_qualify: false,
                    value_table_mode: None,
                    connect_by: None,
                },
            ),
            order_by: None,
            limit: None,
            limit_by: [],
            offset: None,
            fetch: None,
            locks: [],
            for_clause: None,
            settings: None,
            format_clause: None,
        },
    ),
)

[jkosh44]

The following is a more minimal query that repros the same issue:

SELECT SUM(c2) FILTER (WHERE true) FROM test_table;

The issue is here: https://github.com/apache/datafusion-sqlparser-rs/blob/3e94877f03c5b47659093a2d55c3bc04e042013c/src/parser/mod.rs#L1789-L1799

FILTER will only be parsed as a filter if the dialect supports it. I believe by default DF uses GenericDialect, https://github.com/apache/datafusion-sqlparser-rs/blob/main/src/dialect/generic.rs.

GenericDialect does not override the default implementation of supports_filter_during_aggregation(), which returns false,
https://github.com/apache/datafusion-sqlparser-rs/blob/3e94877f03c5b47659093a2d55c3bc04e042013c/src/dialect/mod.rs#L218-L222.

So by default the FILTER clause is not supported, so it's treated as an alias.

[mildbyte]

https://github.com/search?q=repo%3Aapache%2Fdatafusion-sqlparser-rs%20supports_filter_during_aggregation&type=code

So as a workaround we could use any dialect that supports it (e.g. postgresql), gotcha.

[jkosh44]

So as a workaround we could use any dialect that supports it (e.g. postgresql), gotcha.

That sounds like it should work. From some googling it looks like the FILTER clause was added in the 2003 SQL standard, so it sounds reasonable to enable it for the GenericDialect. However, that might be a backwards incompatible change and I'm not sure how those decisions are made for this project.

[alamb]

So as a workaround we could use any dialect that supports it (e.g. postgresql), gotcha.

That sounds like it should work. From some googling it looks like the FILTER clause was added in the 2003 SQL standard, so it sounds reasonable to enable it for the GenericDialect. However, that might be a backwards incompatible change and I'm not sure how those decisions are made for this project.

I don't think we have any specific guidance for this kind of change

[2010YOUY01]

So as a workaround we could use any dialect that supports it (e.g. postgresql), gotcha.

That sounds like it should work. From some googling it looks like the FILTER clause was added in the 2003 SQL standard, so it sounds reasonable to enable it for the GenericDialect. However, that might be a backwards incompatible change and I'm not sure how those decisions are made for this project.

I think supporting a new feature should not have backwards compatibility issue, unlike disabling an existing feature

[jkosh44]

So as a workaround we could use any dialect that supports it (e.g. postgresql), gotcha.

That sounds like it should work. From some googling it looks like the FILTER clause was added in the 2003 SQL standard, so it sounds reasonable to enable it for the GenericDialect. However, that might be a backwards incompatible change and I'm not sure how those decisions are made for this project.

I think supporting a new feature should not have backwards compatibility issue, unlike disabling an existing feature

FWIW, some queries that used to work could start failing due to how FILTER is interpreted. For example, currently with the GenericDialect the following query works fine:

CREATE TABLE t (a int);
SELECT FILTER FROM (SELECT SUM(a) FILTER FROM t);

because FILTER is treated as an alias. However, with the PostgresDialect that query fails with the following error: Schema error: No field named filter. Valid fields are "sum(t.a)"., because FILTER is treated as a keyword.

[jkosh44]

As an aside, it looks like the PostgreSQL dialect and actual PostgreSQL have different behaviors w.r.t. a dangling FILTER clause.

DataFusion CLI v45.0.0
> SET datafusion.sql_parser.dialect = 'PostgreSQL';
0 row(s) fetched. 
Elapsed 0.002 seconds.

> CREATE TABLE t (a INT);
0 row(s) fetched. 
Elapsed 0.006 seconds.

> SELECT SUM(a) FILTER FROM t;
+----------+
| sum(t.a) |
+----------+
| NULL     |
+----------+
1 row(s) fetched. 
Elapsed 0.006 seconds.

psql (17.2 (Ubuntu 17.2-1.pgdg24.04+1))
SSL connection (protocol: TLSv1.3, cipher: TLS_AES_256_GCM_SHA384, compression: off, ALPN: postgresql)
Type "help" for help.

postgres=# CREATE TEMP TABLE t (a INT);
CREATE TABLE
postgres=# SELECT SUM(a) FILTER FROM t;
ERROR:  syntax error at or near "FROM"
LINE 1: SELECT SUM(a) FILTER FROM t;
                             ^
postgres=# 

[Jefffrey]

Filter clause for aggregations is now enabled by default in generic dialect, see #17807 (and its references #16516 and #15719)

e.g. on main:

datafusion-cli$ cargo run
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.22s
     Running `/Users/jeffrey/.cargo_target_cache/debug/datafusion-cli`
DataFusion CLI v50.0.0
> create table test_table(c2 int);
0 row(s) fetched.
Elapsed 0.041 seconds.

> SELECT SUM(c2) FILTER (WHERE true) FROM test_table;
+-------------------------------------------------+
| sum(test_table.c2) FILTER (WHERE Boolean(true)) |
+-------------------------------------------------+
| NULL                                            |
+-------------------------------------------------+
1 row(s) fetched.
Elapsed 0.042 seconds.