Allow MetricsControl to aggregate on a column with an expression

[michellethomas]

In the new MetricsControl, selecting a column with an expression would fail because it's just using the column name which doesn't exist in the underlying table (only in our metadata). This would add the expression (if there is one) inside the aggregation.

@john-bodley @GabeLoins @mistercrunch

[gabe-lyons]

@michellethomas do we need to do something like this for druid as well?

[michellethomas]

I'm not totally sure if you can use a dimension spec (column expression) within a postagg in druid. When I looked through the druid metrics code I didn't see anywhere we were using DruidColumn expression or dimension_spec properties, so it doesn't look like we are doing this now. But I'm not totally sure.

Maybe John or Max know?

[codecov-io]

Codecov Report

Merging #5021 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #5021     +/-   ##
=========================================
+ Coverage   77.35%   77.46%   +0.1%     
=========================================
  Files          44       44             
  Lines        8677     8683      +6     
=========================================
+ Hits         6712     6726     +14     
+ Misses       1965     1957      -8

Impacted Files	Coverage Δ
superset/connectors/sqla/models.py	78.22% <100%> (+1.8%)	⬆️
superset/data/__init__.py	100% <100%> (ø)	⬆️

---

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 ce0011e...f1db6f9. Read the comment docs.

[mistercrunch]

I'm not 100% but I'm pretty sure Druid postaggs have to be made out of metrics.

Your PR LGTM, I'd be great to have a unit test that was failing before and succeeding now. For example we could have a new expression in the "births" dataset that would be called first_letter as substr(name, 1, 1) and do a count distinct of that or something.

[michellethomas]

@mistercrunch I didn't see a lot of tests for this section of the code. Did you have thoughts on where to add tests for this? It looks like I could fit it into viz_tests.TableVizTestCase.

[mistercrunch]

All examples get executed as integration test, so if you add a new example chart that cover this use case it should fail before your PR, succeed after

[michellethomas]

@mistercrunch added tests, thanks for the suggestion!

[mistercrunch]

LGTM

[john-bodley]

Note I believe this only supports one level of indirection, i.e., I'm not certain whether one can create another dimension which leverages num_california in your example.

[john-bodley]

@mistercrunch what are your thoughts about having metrics reference other metrics, or should metric expressions be valid SQL?

[mistercrunch]

Valid SQL was always the assumption, for simplicity and clarity. MetricsControl kind of changes the model a bit though as one might expect to see computed dimensions in there (which is what this fixes). I don't think people would expect to see MetricsControl-generated metrics inside the dropdown though, meaning there should be only one level.