Adding the MetricsControl to the timeseries_limit_metric field #5118
Conversation
michellethomas
force-pushed
the
add_metrics_control_sort_by
branch
from
c1b1b17
to
6f05b48
Compare
Codecov Report
@@ Coverage Diff @@
## master #5118 +/- ##
==========================================
+ Coverage 77.46% 77.47% +<.01%
==========================================
Files 44 44
Lines 8730 8740 +10
==========================================
+ Hits 6763 6771 +8
- Misses 1967 1969 +2
Continue to review full report at Codecov.
|
superset/connectors/druid/models.py
Outdated
| @@ -1148,6 +1148,11 @@ def run_query( # noqa / druid | |||
| metric['column']['type'].upper() == 'FLOAT' | |||
| ): | |||
| metric['column']['type'] = 'DOUBLE' | |||
| if ( | |||
| utils.is_adhoc_metric(timeseries_limit_metric) and | |||
| timeseries_limit_metric['column']['type'].upper() == 'FLOAT' | |||
There was a problem hiding this comment.
I think you also need to add a version check here, see:
https://github.com/apache/incubator-superset/pull/5030/files
There was a problem hiding this comment.
unless this is a different thing?
There was a problem hiding this comment.
The version check is happening a few lines above. https://github.com/apache/incubator-superset/pull/5118/files#diff-a8dd5ec8d8decda2e3c5571d1ec0cdb6R1141
There was a problem hiding this comment.
It's almost the exact same code as a few lines above, maybe refactor a sanitize_metric_object and put as much as possible in there.
[probably out of scope for this PR] I'm starting to think we should make all metrics fit the same model (make predefined metrics look like custom metric objects) as early as possible in the stack and then treat them all the same way.
|
LGTM aside from the comment--- thanks for doing this! |
|
Actually, I need to fix an issue with this on the table viz. |
michellethomas
force-pushed
the
add_metrics_control_sort_by
branch
2 times, most recently
from
2014edb
to
4946384
Compare
michellethomas
force-pushed
the
add_metrics_control_sort_by
branch
from
4946384
to
f8d8ec2
Compare
michellethomas
force-pushed
the
add_metrics_control_sort_by
branch
from
f8d8ec2
to
b380a57
Compare
|
Sorry for the delay, I fixed the issue with Table viz and addressed PR comments. I created an issue to track the task Max suggested that's out of scope for this PR. @GabeLoins @mistercrunch |
|
LGTM |
…l_sort_by Adding the MetricsControl to the timeseries_limit_metric field
Making timeseries_limit_metric (sort by) use the MetricsControl.
@GabeLoins