ktmud commented on pull request #13434: URL: https://github.com/apache/superset/pull/13434#issuecomment-791677215
> 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. I was actually contemplating sending the `orderby` only columns back to client (at least in the Data panel), just so users can have confidence the order by was correctly applied. Currently I'm removing the `orderby` columns with `labels_expected`, but they could also be removed in `query_actions.py`, or be handled by each chart itself. Either way, it seems useful to have the engine always return the sort by field while it's not clear how much the bandwidth saving matters, so I'm inclined to not add this `EngineSpec` parameter just to keep things simple... ---------------------------------------------------------------- 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]
