villebro commented on pull request #13434: URL: https://github.com/apache/superset/pull/13434#issuecomment-791210703
Lots of great improvements here (I'll add review comments later). It seems the root problem here is that the selected column isn't being added to the groupby clause, not the aggregate expression being missing from the select. Also, one thing worthwhile considering is not all db engines require having the order by aggregate expression in the selects: ``` sqlite> select name from birth_names group by name order by sum(num_girls) desc limit 10; Jennifer Jessica Ashley Sarah Amanda Elizabeth Melissa Michelle Kimberly Stephanie sqlite> ``` While I don't see a lot of use cases where omitting the aggregate in the select, it's not strictly always necessary, and could save unnecessary network bandwidth when not needed. So instead of always adding it to the selects, we might want to introduce a parameter in `BaseEngineSpec` stating whether or not orderby aggregates have to be present in the select. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
