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

* [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

* addressing PR comments

* Adding a comment for metrictype values

superset/assets/package.json

@@ -15,8 +15,8 @@
     "dev-server": "webpack-dev-server --mode=development --progress",
     "prod": "node --max_old_space_size=4096 webpack --mode=production --colors --progress",
     "build": "NODE_ENV=production webpack --mode=production --colors --progress",
-    "lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx . && tslint -c tslint.json ./src/**/*.ts{,x}",
-    "lint-fix": "eslint --fix --ignore-path=.eslintignore --ext .js,.jsx . && tslint -c tslint.json --fix ./src/**/*.ts{,x}",
+    "lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx . && tslint -c tslint.json ./{src,spec}/**/*.ts{,x}",
+    "lint-fix": "eslint --fix --ignore-path=.eslintignore --ext .js,.jsx . && tslint -c tslint.json --fix ./{src,spec}/**/*.ts{,x}",
     "sync-backend": "babel-node --presets env src/syncBackend.js",
     "cypress": "cypress",
     "cypress-debug": "cypress open --config watchForFileChanges=true"

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

@@ -0,0 +1,95 @@
+import { ColumnType } from 'src/query/Column';
+import {
+  AdhocMetric, Aggregate, ExpressionType, LABEL_MAX_LENGTH, 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: ColumnType.BIGINT,
+      },
+      expressionType: ExpressionType.SIMPLE,
+    };
+    metrics = new Metrics({
+      ...formData,
+      metric: adhocMetric,
+    });
+    expect(metrics.getMetrics()).toEqual([{
+      aggregate: 'AVG',
+      column: {
+        columnName: 'sum_girls',
+        id: 5,
+        type: ColumnType.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()[0].length).toBeLessThanOrEqual(LABEL_MAX_LENGTH);
+  });
+});

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'});
+  it('should build granularity for sql druid datasources', () => {
+    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,24 @@
+export enum ColumnType {
+  DOUBLE = 'DOUBLE',
+  FLOAT = 'FLOAT',
+  INT = 'INT',
+  BIGINT = 'BIGINT',
+  LONG = 'LONG',
+  REAL = 'REAL',
+  NUMERIC = 'NUMERIC',
+  DECIMAL = 'DECIMAL',
+  MONEY = 'MONEY',
+  DATE = 'DATE',
+  TIME = 'TIME',
+  DATETIME = 'DATETIME',
+  VARCHAR = 'VARCHAR',
+  STRING = 'STRING',
+  CHAR = 'CHAR',
+}
+
+// TODO: fill out additional fields of the Column interface
+export default interface Column {
+  id: number;
+  type: ColumnType;
+  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,100 @@
+import Column from './Column';
+import FormData from './FormData';
+
+export const LABEL_MAX_LENGTH = 43;
+
+// Note that the values of MetricKeys are lower_snake_case because they're
+// used as keys of form data jsons.
+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 <= LABEL_MAX_LENGTH ? label :
+      `${label.substring(0, LABEL_MAX_LENGTH - 3)}...`;
+  }
+}

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