You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/05 23:18:19 UTC
[GitHub] [arrow-datafusion] andygrove opened a new pull request, #2461: Do not allow HAVING clause in non-aggregate queries
andygrove opened a new pull request, #2461:
URL: https://github.com/apache/arrow-datafusion/pull/2461
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes https://github.com/apache/arrow-datafusion/issues/2460
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Neither Postgres nor Spark supports the use of the `HAVING` clause in non-aggregate queries.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Stop allowing the use of `HAVING` in non-aggregate queries.
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
Yes, users will need to use `WHERE` instead of `HAVING` for non-aggregate queries.
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] drusso commented on pull request #2461: Do not allow HAVING clause in non-aggregate queries
Posted by GitBox <gi...@apache.org>.
drusso commented on PR #2461:
URL: https://github.com/apache/arrow-datafusion/pull/2461#issuecomment-1120248968
Hi Andrew and Andy. This behaviour dates back to the [initial](https://github.com/apache/arrow/pull/9364) implementation, so I can try to provide some context. The [select_with_having](https://github.com/apache/arrow-datafusion/blob/6786203ea8c9c4cfe604f74a5cece8e829fb56c8/datafusion/core/src/sql/planner.rs#L2818) and [select_with_having_referencing_column_not_in_select](https://github.com/apache/arrow-datafusion/blob/6786203ea8c9c4cfe604f74a5cece8e829fb56c8/datafusion/core/src/sql/planner.rs#L2829) cases are there, almost certainly, because those queries are valid in MySQL 8. I used both Postgres and MySQL as a reference for the implementation, and in this particular case, opted for the MySQL behaviour, since it's the more permissive of the two. I think since then, Postgres has been adopted as the reference.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #2461: Do not allow HAVING clause in non-aggregate queries
Posted by GitBox <gi...@apache.org>.
andygrove commented on PR #2461:
URL: https://github.com/apache/arrow-datafusion/pull/2461#issuecomment-1120443793
Thanks @drusso that is helpful. I think I will go ahead and close this PR in the next day or two unless anyone objects. Moving back to draft for now.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #2461: Do not allow HAVING clause in non-aggregate queries
Posted by GitBox <gi...@apache.org>.
andygrove commented on PR #2461:
URL: https://github.com/apache/arrow-datafusion/pull/2461#issuecomment-1119823525
> I don't mind this change, but I think technically postgres supports HAVING in queries without aggregates:
>
> ```sql
> ❯ select * from andrew having substr(location, 4) = 'et_sound';
> +-----------------+-------------+-------+-----------------+----------------+------------+
> | measurement | location | state | surface_degrees | bottom_degrees | timestamp |
> +-----------------+-------------+-------+-----------------+----------------+------------+
> | h2o_temperature | puget_sound | WA | 55.8 | 40.2 | 1568756160 |
> | h2o_temperature | puget_sound | WA | 55.8 | 40.2 | 1600756160 |
> +-----------------+-------------+-------+-----------------+----------------+------------+
> 2 rows in set. Query took 0.013 seconds.
> ```
hmm ok maybe we should not make this change then. Here is the test I was running in postgres:
```
postgres=# select id, name from book having price < 6;
ERROR: column "book.id" must appear in the GROUP BY clause or be used in an aggregate function
```
This was with postgres 14.2
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #2461: Do not allow HAVING clause in non-aggregate queries
Posted by GitBox <gi...@apache.org>.
andygrove commented on PR #2461:
URL: https://github.com/apache/arrow-datafusion/pull/2461#issuecomment-1119848904
> > hmm ok maybe we should not make this change then. Here is the test I was running in postgres:
>
> Ah, that makes sense as `price` wasn't in the select list (which is technically supposed to be evaluated before the HAVING)
>
> I bet this would work:
>
> ```sql
> select id, name, price from book having price < 6;
> ```
That also fails for me:
```
postgres=# select id, name, price from book having price < 6;
ERROR: column "book.id" must appear in the GROUP BY clause or be used in an aggregate function
LINE 1: select id, name, price from book having price < 6;
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #2461: Do not allow HAVING clause in non-aggregate queries
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2461:
URL: https://github.com/apache/arrow-datafusion/pull/2461#issuecomment-1119833519
> hmm ok maybe we should not make this change then. Here is the test I was running in postgres:
Ah, that makes sense as `price` wasn't in the select list (which is technically supposed to be evaluated before the HAVING)
I bet this would work:
```sql
select id, name, price from book having price < 6;
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jdye64 commented on pull request #2461: Do not allow HAVING clause in non-aggregate queries
Posted by GitBox <gi...@apache.org>.
jdye64 commented on PR #2461:
URL: https://github.com/apache/arrow-datafusion/pull/2461#issuecomment-1119163039
Seems [test](https://github.com/apache/arrow-datafusion/blob/cc496f87caf8d13e4c17025e0d3b86643a21a243/datafusion/core/tests/sql/group_by.rs#L194) needs to be updated to expect the error
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #2461: Do not allow HAVING clause in non-aggregate queries
Posted by GitBox <gi...@apache.org>.
andygrove commented on PR #2461:
URL: https://github.com/apache/arrow-datafusion/pull/2461#issuecomment-1119169178
> LGTM - The logic `!group_by_exprs.is_empty() || !aggr_exprs.is_empty()` seems to allow for `HAVING` clauses if aggregate expressions are present, however might it be worth adding another test that does have an aggregate and `HAVING` clause? Could be another simple check in an existing test even.
Yes that's a great point. I checked and there is already a test to cover this case:
``` rust
#[test]
fn select_aggregate_with_having_that_reuses_aggregate() {
let sql = "SELECT MAX(age)
FROM person
HAVING MAX(age) < 30";
let expected = "Projection: #MAX(person.age)\
\n Filter: #MAX(person.age) < Int64(30)\
\n Aggregate: groupBy=[[]], aggr=[[MAX(#person.age)]]\
\n TableScan: person projection=None";
quick_test(sql, expected);
}
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #2461: Do not allow HAVING clause in non-aggregate queries
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2461:
URL: https://github.com/apache/arrow-datafusion/pull/2461#issuecomment-1120414209
Thank you for the context @drusso -- I do not have a strong preference one way or the other in terms of switching behaviors / merging this PR
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #2461: Do not allow HAVING clause in non-aggregate queries
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2461:
URL: https://github.com/apache/arrow-datafusion/pull/2461#issuecomment-1119852214
> That also fails for me:
I am stumped 🤔
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove closed pull request #2461: Do not allow HAVING clause in non-aggregate queries
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #2461: Do not allow HAVING clause in non-aggregate queries
URL: https://github.com/apache/arrow-datafusion/pull/2461
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org