[SIP-5] Build metrics in query_object in the client #6423

xtinec · 2018-11-22T03:24:04Z

Unify the metric interface (absorb the current plain string metric for built-in metric keys into the format used by adhoc metric)
Port the logic in adhocMetric on the client and process_metrics in the backend to the new typed Metrics class
Omit hasCustomLabel and formFromData properties from the new metric interface as their value can be inferred from label and optionName
Expose from the Metrics class both metrics and their labels as public methods to match the all_metrics and metric_labels fields in the backend code
Provide defaut values for filters, metrics and groupby in the backend
@kristw @williaster @conglei

codecov-io · 2018-11-22T03:36:04Z

Codecov Report

Merging #6423 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #6423   +/-   ##
=======================================
  Coverage   73.31%   73.31%           
=======================================
  Files          67       67           
  Lines        9591     9591           
=======================================
  Hits         7032     7032           
  Misses       2559     2559

Impacted Files	Coverage Δ
superset/common/query_object.py	`95.23% <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 2731a01...a4597af. Read the comment docs.

williaster

looks great overall, had just a couple small comments! 😎

williaster · 2018-11-27T19:20:12Z

superset/assets/spec/javascripts/superset-ui/buildQueryObject.test.ts

  it('should build granularity for sql alchemy datasources', () => {
-    const query = build({datasource: '5__table', granularity_sqla: 'ds'});
+    query = build({datasource: '5__table', granularity_sqla: 'ds'});
    expect(query.granularity).toEqual('ds');
  });

  it('should build granularity for sql alchemy datasources', () => {


should this say druid?

Good catch! Fixing. 🔨

williaster · 2018-11-27T19:21:28Z

superset/assets/src/query/Column.ts

+// TODO: fill out additional fields of the Column interface
+export default interface Column {
+  id: number;
+  type: string;


do we have enough information to enumerate these yet or string best for now?

I think so. Found these in connectors/base/models.py

num_types = ( 'DOUBLE', 'FLOAT', 'INT', 'BIGINT', 'LONG', 'REAL', 'NUMERIC', 'DECIMAL', 'MONEY', ) date_types = ('DATE', 'TIME', 'DATETIME') str_types = ('VARCHAR', 'STRING', 'CHAR')

I can make this an enum. 🙌

williaster · 2018-11-27T19:33:18Z

superset/assets/src/query/Metric.ts

+import FormData from './FormData';
+
+export enum MetricKey {
+  METRIC = 'metric',


nit -- could we make the key + values match / be UPPER_CASE in all of the enums? there's a mix now.

You mean making it SECONDARY_METRIC = 'SECONDARY_METRIC'?
Yeah, we can. Just need to change the values hardcoded on the server side. It is currently expecting lower_snake_case like secondary_metric.

hrm, I'm thinking maybe we should leave the values as lower_snake_case as they're used in the form_data json (example below). It may be odd to have UPPER_CASE keys in the json. Thoughts?

"secondary_metric": { "type": "SelectControl", "label": "Color Metric", "default": null, "description": "A metric to use for color" }

ah that makes sense, I'm okay with it for that reason but can we add a comment explaining why they keys/values have different casing?

Yup. Adding.

williaster · 2018-11-27T19:36:35Z

superset/assets/src/query/Metric.ts

+
+export type AdhocMetric = {
+  label?: string,
+  optionName?: string,


do we know what optionName is? sounds redundant with label 🤔

The naming is very confusing. It is either provided via adhocMetric or generated (e.g. metric_vgops097wej_g8uff99zhk7).
https://github.com/apache/incubator-superset/blob/4579b12732f975f2d278476975d0f876ab470ae1/superset/assets/src/explore/AdhocMetric.js#L48-L50

ah, so it's more like an id, hmmmm. guess we can leave since it prob would require a db migration 😣

williaster · 2018-11-27T19:47:32Z

superset/assets/src/query/Metric.ts

+    } else {
+      label = metric.sqlExpression;
+    }
+    return label.length < 43 ? label : `${label.substring(0, 40)}...`;


could we define 43 as a const?

williaster · 2018-11-27T19:48:22Z

superset/assets/spec/javascripts/superset-ui/Metric.test.ts

+      ...formData,
+      metric: adhocMetric,
+    });
+    expect(metrics.getLabels()).toEqual(['COUNT(verrrrrrrrry_loooooooooooooooooooo...']);


might be more robust to assert that length is less than the full length in the case that someone changes the char limit? (or they can just update the test, too 😄 )

williaster · 2018-11-27T20:56:46Z

superset/common/query_object.py

-            groupby: List[str],
-            metrics: List[Metric],
-            filters: List[str],
+            groupby: List[str] = None,


any reason to not just default these to [] instead of None since we do below anyway?

In python, setting the default value as [] violates lint rules, and this is recommended way to do so.

- Unify the metric interface (absorb the current plain string metric for built-in metric keys into the format used by adhoc metric) - Port the logic in adhocMetric on the client and process_metrics in the backend to the new typed Metrics class - Omit hasCustomLabel and formFromData properties from the new metric interface as their value can be inferred from label and optionName - Expose from the Metrics class both metrics and their labels as public methods to match the all_metrics and metric_labels fields in the backend code - Provide defaut values for filters, metrics and groupby in the backend

williaster · 2018-11-29T18:13:54Z

superset/assets/src/query/Metric.ts

+
+type Metric = {
+  label: string;
+} & Partial<AdhocMetric>;


oh weird, my comment here got lost.

Should this instead be a | AdhocMetric instead of & Partial<AdhocMetric>? it seems like a Metric is either a saved metric (with just a required label) or an AdhocMetric with required expressionType and either sqlExpression or column+aggregate.

My making it a Partial aren't we losing the required AdhocMetric fields?

Nope, Partial here makes AdhocMetrics optional in its entirety (we can test this by removing required fields like aggregate for the Simple expressionType columns in our unit tests and we are seeing type errors 😄). Since we are now requiring label field, this also makes label a required field in the resulting Metrics we return from getMetrics().

We could alternatively write this as {label: string} | (AdhocMetric & { label: string })

Gotcha, {label: string} | (AdhocMetric & { label: string }) makes sense based on my understanding.

Partial here makes AdhocMetrics optional in its entirety

I thought that Partial made all fields optional, but if you do indeed get type errors upon removing aggregate, etc. I'm okay with this way of writing it 👍. Can you verify that and then we can merge (unless @kristw or @conglei have feedback)

Yep. Verified that in the unit tests by removing aggregate.
Btw, Partial does make all fields optional (it is what I meant by "optional in its entirety"), so your understanding on Partial is definitely correct. 😄

williaster

This lgtm with the one question about the Metric type (| vs &)

williaster reviewed Nov 27, 2018

View reviewed changes

xtinec force-pushed the query-object-metrics branch 2 times, most recently from 58f7f00 to 2940853 Compare November 29, 2018 00:32

xtinec and others added 2 commits November 28, 2018 19:39

addressing PR comments

a4597af

xtinec force-pushed the query-object-metrics branch from 2940853 to a4597af Compare November 29, 2018 00:39

williaster reviewed Nov 29, 2018

View reviewed changes

williaster approved these changes Nov 29, 2018

View reviewed changes

Adding a comment for metrictype values

e06f873

kristw added the #code-quality label Nov 30, 2018

williaster merged commit 5f7817a into apache:master Nov 30, 2018

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIP-5] Build metrics in query_object in the client #6423

[SIP-5] Build metrics in query_object in the client #6423

xtinec commented Nov 22, 2018

codecov-io commented Nov 22, 2018 •

edited

williaster left a comment

williaster Nov 27, 2018

xtinec Nov 28, 2018

williaster Nov 27, 2018

xtinec Nov 28, 2018

williaster Nov 27, 2018

xtinec Nov 28, 2018 •

edited

xtinec Nov 28, 2018

williaster Nov 29, 2018

xtinec Nov 29, 2018

williaster Nov 27, 2018

xtinec Nov 28, 2018

williaster Nov 29, 2018

williaster Nov 27, 2018

xtinec Nov 28, 2018

williaster Nov 27, 2018

xtinec Nov 28, 2018

williaster Nov 27, 2018

conglei Nov 27, 2018

williaster Nov 29, 2018

xtinec Nov 29, 2018

williaster Nov 29, 2018

xtinec Nov 29, 2018

williaster left a comment

[SIP-5] Build metrics in query_object in the client #6423

[SIP-5] Build metrics in query_object in the client #6423

Conversation

xtinec commented Nov 22, 2018

codecov-io commented Nov 22, 2018 • edited

Codecov Report

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtinec Nov 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 22, 2018 •

edited

xtinec Nov 28, 2018 •

edited