fix(query): order by adhoc metrics should trigger group by

[ktmud]

SUMMARY

Update SQL query generator to properly handle adhoc metrics in order by. The idea is collect adhoc metrics in orderby before deciding whether to apply groupby. Closes #13465 .

Also assume it is a GROUP BY query unless metrics to set to None. Depends on: apache-superset/superset-ui#995 (released in @superset-ui/core v0.17.18)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

Correctly generates query for group by and sort by without metrics.

TEST PLAN

Unit tests and a new Cypress test

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[villebro]

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.

[ktmud]

Thanks for the headsup, @villebro !

Another thing I noticed is that it's not always correct to assume "columns" with no "metrics" requires no aggregation. E.g. when table chart selects groupby with no metrics in aggregation mode, the output should be unique combination of the groups instead of raw records. This is mentioned as the second problem in #13228 .

Therefore we can not really deprecate groupby in QueryObject. Or we will need to add at least an is_aggregate flag if groupby is to be cleaned up.

[villebro]

Thanks for the headsup, @villebro !

Another thing I noticed is that it's not always correct to assume "columns" with no "metrics" requires no aggregation. E.g. when table chart selects groupby with no metrics in aggregation mode, the output should be unique combination of the groups instead of raw records. This is mentioned as the second problem in #13228 .

Therefore we can not really deprecate groupby in QueryObject. Or we will need to add at least an is_aggregate flag is groupby is to be cleaned up.

Yes, I've been planning on that - I was thinking along the lines of an aggregation_type, which could be none, distinct and groupby, where any aggregate expression would force groupby, but in the absence of aggregates, we would default to none, but would provide the option to do distinct.

[ktmud]

I'm trying to imagine what would be a useful case in exposing SELECT DISTINCT abc FROM tbl in the API and why would the user (or client query builder) choose one way or another---since it generally produces the same results as group by (SELECT abc FROM tbl GROUP BY abc).

Note that in some data engines (e.g. Presto), there are performance implications in using DISTINCT vs GROUP BY (and GROUP BY is normally faster).

I'm OK with either way, but would hope we can keep the interface simple and avoid the possibility of getting the same results with different query configs.

[ktmud]

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...

[villebro]

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...

I'm not super passionate about adding distinct support, so if there's any risk of it causing added complexity or getting in the way I'm ok with just supporting group bys. Same goes for adding vs removing order bys from the select - I'm fine adding them in the select, and agree that it's good to have them around for validation purposes (the real optimizations are probably elsewhere anyways, like using binary protocols, streaming etc). But in general I'm in the camp of "if a feature can easily be added, why not", and tend to prefer to avoid adding limitations or forcing features if there are no clear motivations for doing so.

[ktmud]

I just removed the logic of adding order by only metrics to select expressions and it still works at least for Postgres. We can decide if we want to expose these hidden columns later when there are clearer actual feature requests.

[codecov]

Codecov Report

Merging #13434 (7eea891) into master (98a26e7) will decrease coverage by 4.78%.
The diff coverage is 84.76%.

@@            Coverage Diff             @@
##           master   #13434      +/-   ##
==========================================
- Coverage   77.41%   72.63%   -4.79%     
==========================================
  Files         918      918              
  Lines       46673    46674       +1     
  Branches     5720     5720              
==========================================
- Hits        36132    33901    -2231     
- Misses      10405    12557    +2152     
- Partials      136      216      +80     

Flag	Coverage Δ
cypress	?
hive	?
javascript	63.16% <0.00%> (-0.01%)	⬇️
mysql	80.55% <89.00%> (+<0.01%)	⬆️
postgres	?
presto	80.29% <87.00%> (+<0.01%)	⬆️
python	80.80% <89.00%> (-0.31%)	⬇️
sqlite	80.22% <89.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...tend/src/explore/components/DisplayQueryButton.jsx	43.75% <0.00%> (-32.06%)	⬇️
superset/viz.py	56.07% <0.00%> (-0.04%)	⬇️
superset/common/query_actions.py	92.85% <75.00%> (-2.53%)	⬇️
superset/connectors/sqla/models.py	90.50% <89.06%> (-0.13%)	⬇️
superset/common/query_context.py	82.06% <100.00%> (ø)
superset/common/query_object.py	90.47% <100.00%> (-0.07%)	⬇️
superset/connectors/base/models.py	90.81% <100.00%> (+0.03%)	⬆️
superset/db_engine_specs/base.py	86.19% <100.00%> (+0.03%)	⬆️
superset/db_engine_specs/pinot.py	95.12% <100.00%> (+0.12%)	⬆️
superset/models/core.py	89.37% <100.00%> (ø)
... and 228 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98a26e7...7eea891. Read the comment docs.

[ktmud]

So I think there is still a way to fully deprecate groupby. We just assume group by queries whenever metrics is not None. An empty metrics array should also trigger group by.

This could be a somewhat dangerous change and would require more testing.

[ktmud]

This function is not used by this PR anymore, but I'm keeping it here for convenience.

[ktmud]

Use x in case some may wonder whether metric is also from the parent context (e.g. an __init__ argument).

[villebro]

nit: I'd personally prefer metric_ here

[ktmud]

I always prefer x in list comprehensions and lambda functions because it's simpler and reads more "local".

[ktmud]

Presto tests seem to be failing because of this error:

• [Explore]Ensure ad-hoc metrics are not named the same as an existing column #13426
• Invalid plan for ORDER BY expression that refers to output column prestodb/presto#4698