You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "goldmedal (via GitHub)" <gi...@apache.org> on 2025/04/17 13:12:03 UTC
[PR] enable `supports_filter_during_aggregation` for Generic [datafusion-sqlparser-rs]
goldmedal opened a new pull request, #1815:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1815
- Related to https://github.com/apache/datafusion/issues/15719
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] enable `supports_filter_during_aggregation` for Generic dialect [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1815:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1815#issuecomment-2813825103
> oops , I forgot to mention the tests. I found it will be covered by the original case:
I was thinking a test that would prevent against regressions in a future refactor -- namely a test that will fail if this code change is reverted,
I think as it stands, if the code change is reverted all the tests will still pass 🤔
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] enable `supports_filter_during_aggregation` for Generic dialect [datafusion-sqlparser-rs]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on code in PR #1815:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1815#discussion_r2050635516
##########
tests/sqlparser_common.rs:
##########
@@ -11642,16 +11642,28 @@ fn parse_connect_by() {
#[test]
fn test_selective_aggregation() {
+ let testing_dialects = all_dialects_where(|d| d.supports_filter_during_aggregation());
+ let expected_dialects: Vec<Box<dyn Dialect>> = vec![
+ Box::new(PostgreSqlDialect {}),
+ Box::new(DatabricksDialect {}),
+ Box::new(HiveDialect {}),
+ Box::new(SQLiteDialect {}),
+ Box::new(DuckDbDialect {}),
+ Box::new(GenericDialect {}),
+ ];
+ assert_eq!(testing_dialects.dialects.len(), expected_dialects.len());
+ expected_dialects
+ .into_iter()
+ .for_each(|d| assert!(d.supports_filter_during_aggregation()));
Review Comment:
I list all the dialects expected to support filtering during aggregation.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] enable `supports_filter_during_aggregation` for Generic dialect [datafusion-sqlparser-rs]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #1815:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1815#issuecomment-2814306305
> > oops , I forgot to mention the tests. I found it will be covered by the original case:
>
> I was thinking a test that would prevent against regressions in a future refactor -- namely a test that will fail if this code change is reverted,
>
> I think as it stands, if the code change is reverted all the tests will still pass 🤔
I see. Make sense. I'll add another test to protect it.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] enable `supports_filter_during_aggregation` for Generic dialect [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #1815:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1815
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] enable `supports_filter_during_aggregation` for Generic [datafusion-sqlparser-rs]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #1815:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1815#issuecomment-2812858094
@alamb, What do you think about this? I think this change is related to the default behavior of DataFusion.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] enable `supports_filter_during_aggregation` for Generic dialect [datafusion-sqlparser-rs]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #1815:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1815#issuecomment-2812897010
> The only thing I think this PR needs is a test and it should be good to go
oops , I forgot to mention the tests. I found it will be covered by the original case:
https://github.com/apache/datafusion-sqlparser-rs/blob/81d8909e006f90c9b2f06fdeb4bab22935bec827/tests/sqlparser_common.rs#L11644-L11655
I guess it's enough?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] enable `supports_filter_during_aggregation` for Generic dialect [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1815:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1815#issuecomment-2812879909
Makes sense to me @goldmedal -- I think the intent is that the GenericDialect should support as many SQL constructs as possible
The only thing I think this PR needs is a test and it should be good to go
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org