gortiz commented on PR #19554: URL: https://github.com/apache/pinot/pull/19554#issuecomment-5700582345
Good catch — yes, and they were worse than merely allowed. Both compiled and then failed during broker reduce: ``` Caught exception while reducing data tables: Failed to find SELECT expression: amount in the GROUP-BY clause ``` An internal error naming a clause the user did not write, and in the first query naming a GROUP BY clause that is not even there. A third shape had it too: `SELECT city, COUNT(*) FROM t GROUP BY city HAVING COUNT(*) > amount`. The cause: I was validating with `expressionOutsideGroupByList()`, which accepts an expression as soon as it *contains* an aggregation anywhere. `COUNT(*) > amount` contains `COUNT(*)`, so the whole predicate was waved through and `amount` was never checked. Fixed in cf6ed8a. `findUngroupedReference()` walks the predicate and stops *at* an aggregation rather than short-circuiting on one, so an identifier is accepted only when it is a grouping column or an argument of an aggregation. It returns the offending expression, so the message now names the column instead of the whole predicate. Two things it deliberately still accepts: - a column inside an aggregate — `HAVING SUM(amount) > MIN(amount)`; - a filtered aggregation — `HAVING COUNT(*) FILTER(WHERE INT_COL < 5) > 0`. The FILTER predicate is evaluated per row while aggregating, so it may reference ungrouped columns. `FilteredAggregationsTest` caught this when I first got it wrong. Regression tests added for all three of the rejected shapes plus both accepted ones. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
