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

- 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

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

@@ -0,0 +1,92 @@
+import { AdhocMetric, Aggregate, ExpressionType, Metrics } from 'src/query/Metric';
+
+describe('Metrics', () => {
+  let metrics: Metrics;
+  const formData = {
+    datasource: '5__table',
+    granularity_sqla: 'ds',
+  };
+
+  it('should build metrics for built-in metric keys', () => {
+    metrics = new Metrics({
+      ...formData,
+      metric: 'sum__num',
+    });
+    expect(metrics.getMetrics()).toEqual([{label: 'sum__num'}]);
+    expect(metrics.getLabels()).toEqual(['sum__num']);
+  });
+
+  it('should build metrics for simple adhoc metrics', () => {
+    const adhocMetric: AdhocMetric = {
+      aggregate: Aggregate.AVG,
+      column: {
+        columnName: 'sum_girls',
+        id: 5,
+        type: 'BIGINT',
+      },
+      expressionType: ExpressionType.SIMPLE,
+    };
+    metrics = new Metrics({
+      ...formData,
+      metric: adhocMetric,
+    });
+    expect(metrics.getMetrics()).toEqual([{
+      aggregate: 'AVG',
+      column: {
+        columnName: 'sum_girls',
+        id: 5,
+        type: 'BIGINT',
+      },
+      expressionType: 'SIMPLE',
+      label: 'AVG(sum_girls)',
+    }]);
+    expect(metrics.getLabels()).toEqual(['AVG(sum_girls)']);
+  });
+
+  it('should build metrics for SQL adhoc metrics', () => {
+    const adhocMetric: AdhocMetric = {
+      expressionType: ExpressionType.SQL,
+      sqlExpression: 'COUNT(sum_girls)',
+    };
+    metrics = new Metrics({
+      ...formData,
+      metric: adhocMetric,
+    });
+    expect(metrics.getMetrics()).toEqual([{
+      expressionType: 'SQL',
+      label: 'COUNT(sum_girls)',
+      sqlExpression: 'COUNT(sum_girls)',
+    }]);
+    expect(metrics.getLabels()).toEqual(['COUNT(sum_girls)']);
+  });
+
+  it('should build metrics for adhoc metrics with custom labels', () => {
+    const adhocMetric: AdhocMetric = {
+      expressionType: ExpressionType.SQL,
+      label: 'foo',
+      sqlExpression: 'COUNT(sum_girls)',
+    };
+    metrics = new Metrics({
+      ...formData,
+      metric: adhocMetric,
+    });
+    expect(metrics.getMetrics()).toEqual([{
+      expressionType: 'SQL',
+      label: 'foo',
+      sqlExpression: 'COUNT(sum_girls)',
+    }]);
+    expect(metrics.getLabels()).toEqual(['foo']);
+  });
+
+  it('should truncate labels if they are too long', () => {
+    const adhocMetric: AdhocMetric = {
+      expressionType: ExpressionType.SQL,
+      sqlExpression: 'COUNT(verrrrrrrrry_loooooooooooooooooooooong_string)',
+    };
+    metrics = new Metrics({
+      ...formData,
+      metric: adhocMetric,
+    });
+    expect(metrics.getLabels()).toEqual(['COUNT(verrrrrrrrry_loooooooooooooooooooo...']);
+  });
+});

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

@@ -1,13 +1,24 @@
-import build from 'src/query/buildQueryObject';
+import build, { QueryObject } from 'src/query/buildQueryObject';
 
 describe('queryObjectBuilder', () => {
+  let query: QueryObject;
+
   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', () => {
-    const query = build({datasource: '5__druid', granularity: 'ds'});
+    query = build({datasource: '5__druid', granularity: 'ds'});
     expect(query.granularity).toEqual('ds');
   });
+
+  it('should build metrics', () => {
+    query = build({
+      datasource: '5__table',
+      granularity_sqla: 'ds',
+      metric: 'sum__num',
+    });
+    expect(query.metrics).toEqual([{label: 'sum__num'}]);
+  });
 });

superset/assets/src/query/Column.ts

@@ -0,0 +1,6 @@
+// TODO: fill out additional fields of the Column interface
+export default interface Column {
+  id: number;
+  type: string;
+  columnName: string;
+}

superset/assets/src/query/FormData.ts

@@ -1,17 +1,26 @@
+import { AdhocMetric, MetricKey } from './Metric';
+
 // Type signature and utility functions for formData shared by all viz types
 // It will be gradually filled out as we build out the query object
-interface BaseFormData {
+
+// Define mapped type separately to work around a limitation of TypeScript
+// https://github.com/Microsoft/TypeScript/issues/13573
+// The Metrics in formData is either a string or a proper metric. It will be
+// unified into a proper Metric type during buildQuery (see `/query/Metrics.ts`).
+type Metrics = Partial<Record<MetricKey, AdhocMetric | string>>;
+
+type BaseFormData = {
   datasource: string;
-}
+} & Metrics;
 
 // FormData is either sqla-based or druid-based
-interface SqlaFormData extends BaseFormData {
+type SqlaFormData = {
   granularity_sqla: string;
-}
+} & BaseFormData;
 
-interface DruidFormData extends BaseFormData {
+type DruidFormData = {
   granularity: string;
-}
+} & BaseFormData;
 
 type FormData = SqlaFormData | DruidFormData;
 export default FormData;

superset/assets/src/query/Metric.ts

@@ -0,0 +1,95 @@
+import Column from './Column';
+import FormData from './FormData';
+
+export enum MetricKey {
+  METRIC = 'metric',
+  METRICS = 'metrics',
+  PERCENT_METRICS = 'percent_metrics',
+  RIGHT_AXIS_METRIC = 'metric_2',
+  SECONDARY_METRIC = 'secondary_metric',
+  X = 'x',
+  Y = 'y',
+  SIZE = 'size',
+}
+
+export enum Aggregate {
+  AVG = 'AVG',
+  COUNT = 'COUNT ',
+  COUNT_DISTINCT = 'COUNT_DISTINCT',
+  MAX = 'MAX',
+  MIN = 'MIN',
+  SUM = 'SUM',
+}
+
+export enum ExpressionType {
+  SIMPLE = 'SIMPLE',
+  SQL = 'SQL',
+}
+
+interface AdhocMetricSimple {
+  expressionType: ExpressionType.SIMPLE;
+  column: Column;
+  aggregate: Aggregate;
+}
+
+interface AdhocMetricSQL {
+  expressionType: ExpressionType.SQL;
+  sqlExpression: string;
+}
+
+export type AdhocMetric = {
+  label?: string,
+  optionName?: string,
+} & (AdhocMetricSimple | AdhocMetricSQL);
+
+type Metric = {
+  label: string;
+} & Partial<AdhocMetric>;
+
+export default Metric;
+
+export class Metrics {
+  // Use Array to maintain insertion order for metrics that are order sensitive
+  private metrics: Metric[];
+
+  constructor(formData: FormData) {
+    this.metrics = [];
+    for (const key of Object.keys(MetricKey)) {
+      const metric = formData[MetricKey[key] as MetricKey];
+      if (metric) {
+        if (typeof metric === 'string') {
+          this.metrics.push({
+            label: metric,
+          });
+        } else {
+          // Note we further sanitize the metric label for BigQuery datasources
+          // TODO: move this logic to the client once client has more info on the
+          // the datasource
+          const label = metric.label || this.getDefaultLabel(metric);
+          this.metrics.push({
+            ...metric,
+            label,
+          });
+        }
+      }
+    }
+  }
+
+  public getMetrics() {
+    return this.metrics;
+  }
+
+  public getLabels() {
+    return this.metrics.map((m) => m.label);
+  }
+
+  private getDefaultLabel(metric: AdhocMetric) {
+    let label: string;
+    if (metric.expressionType === ExpressionType.SIMPLE) {
+      label = `${metric.aggregate}(${(metric.column.columnName)})`;
+    } else {
+      label = metric.sqlExpression;
+    }
+    return label.length < 43 ? label : `${label.substring(0, 40)}...`;
+  }
+}

superset/assets/src/query/buildQueryObject.ts

@@ -1,9 +1,11 @@
 import FormData, { getGranularity } from './FormData';
+import Metric, { Metrics } from './Metric';
 
 // TODO: fill out the rest of the query object
 export interface QueryObject {
   granularity: string;
   groupby?: string[];
+  metrics?: Metric[];
 }
 
 // Build the common segments of all query objects (e.g. the granularity field derived from
@@ -14,5 +16,6 @@ export interface QueryObject {
 export default function buildQueryObject<T extends FormData>(formData: T): QueryObject {
   return {
     granularity: getGranularity(formData),
+    metrics: new Metrics(formData).getMetrics(),
   };
 }

superset/common/query_object.py

@@ -1,12 +1,12 @@
 # pylint: disable=R
-from typing import Dict, List, Optional, Union
+from typing import Dict, List, Optional
 
 from superset import app
 from superset.utils import core as utils
 
 # TODO: Type Metrics dictionary with TypedDict when it becomes a vanilla python type
 # https://github.com/python/mypy/issues/5288
-Metric = Union[str, Dict]
+Metric = Dict
 
 
 class QueryObject:
@@ -17,9 +17,9 @@ class QueryObject:
     def __init__(
             self,
             granularity: str,
-            groupby: List[str],
-            metrics: List[Metric],
-            filters: List[str],
+            groupby: List[str] = None,
+            metrics: List[Metric] = None,
+            filters: List[str] = None,
             time_range: Optional[str] = None,
             time_shift: Optional[str] = None,
             is_timeseries: bool = False,
@@ -32,10 +32,10 @@ def __init__(
         self.granularity = granularity
         self.from_dttm, self.to_dttm = utils.get_since_until(time_range, time_shift)
         self.is_timeseries = is_timeseries
-        self.groupby = groupby
-        self.metrics = metrics
+        self.groupby = groupby or []
+        self.metrics = metrics or []
+        self.filter = filters or []
         self.row_limit = row_limit
-        self.filter = filters
         self.timeseries_limit = int(limit)
         self.timeseries_limit_metric = timeseries_limit_metric
         self.order_desc = order_desc