SQL: Enhance error message on filtering check against aggs #68763
Conversation
Distinguish between the case where the filtering is a WHERE with aggs appart from a HAVING with missing aggs.
|
Pinging @elastic/es-ql (Team:QL) |
| } else { | ||
| localFailures.add( | ||
| fail(e, "Cannot use WHERE filtering on aggregate function [{}], use HAVING instead", Expressions.name(e))); | ||
|
|
|
|
||
| assertEquals("1:35: [int] field must appear in the GROUP BY clause or be used in an aggregate function", | ||
| error("SELECT int FROM test HAVING int = count(1)")); | ||
| // Note: "SELECT int FROM test HAVING int = 1" works, though it normally shouldn't; to correct this out, we'd need to qualify the |
There was a problem hiding this comment.
Maybe add a test with accept(<query>) and a TODO? to have it more clear?
There was a problem hiding this comment.
+1. I'd like to see a few more tests with it working and not working especially when dealing with non-naked expressions such as field inside a scalar function.
There was a problem hiding this comment.
A few more queries added under a new test.
| } | ||
|
|
||
| public void testHavingInAggs() { | ||
| assertEquals("1:29: [int] field must appear in the GROUP BY clause or be used in an aggregate function", |
There was a problem hiding this comment.
Good catch for queries with implicit grouping.
| fail(e, "Cannot use WHERE filtering on aggregate function [{}], use HAVING instead", Expressions.name(e))); | ||
| if (filterChild instanceof Project) { | ||
| filter.condition().forEachDown(FieldAttribute.class, | ||
| fa -> localFailures.add( |
There was a problem hiding this comment.
Slight adjustment " or in an aggregate function" (drop be used)
|
|
||
| assertEquals("1:35: [int] field must appear in the GROUP BY clause or be used in an aggregate function", | ||
| error("SELECT int FROM test HAVING int = count(1)")); | ||
| // Note: "SELECT int FROM test HAVING int = 1" works, though it normally shouldn't; to correct this out, we'd need to qualify the |
There was a problem hiding this comment.
+1. I'd like to see a few more tests with it working and not working especially when dealing with non-naked expressions such as field inside a scalar function.
- error message slight adjustment - adding one more test.
| public void testHavingAsWhere() { | ||
| // TODO: this query works, though it normally shouldn't; a check about it could only be enforced if the Filter would be qualified | ||
| // (WHERE vs HAVING). Otoh, this "extra flexibility" shouldn't be harmful atp. | ||
| accept("SELECT int FROM test HAVING int = 1"); |
6 participants
This PR introduces a distinction between the failures caused by WHERE-filtering with aggs
vs HAVING-filtering with missing aggs and improving the error messaging around that.
Closes #57125.