[Chart]Unify Metric format (#63)

* unify the metric format * fix test * fix lint * fix lint * change per comments in pr
apache · Nov 26, 2021 · 9c8c0a9 · 9c8c0a9
1 parent fcb7ded
commit 9c8c0a9
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 19 deletions.
diff --git a/...frontend/temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/query/Metric.ts b/...frontend/temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/query/Metric.ts
@@ -26,34 +26,37 @@ export enum Aggregate {
 }
 
 export enum ExpressionType {
+  BUILTIN = 'BUILTIN',
   SIMPLE = 'SIMPLE',
   SQL = 'SQL',
 }
 
-interface AdhocMetricSimple {
+interface SimpleMetric {
   expressionType: ExpressionType.SIMPLE;
   column: Column;
   aggregate: Aggregate;
+  label?: string;
+  optionName?: string;
 }
 
-interface AdhocMetricSQL {
+interface SQLMetric {
   expressionType: ExpressionType.SQL;
   sqlExpression: string;
-}
-
-export type AdhocMetric = {
   label?: string;
   optionName?: string;
-} & (AdhocMetricSimple | AdhocMetricSQL);
+}
+
+interface BuiltInMetric {
+  expressionType: ExpressionType.BUILTIN;
+  label: string;
+}
 
 // Type of metrics in form data
-export type FormDataMetric = string | AdhocMetric;
+export type FormDataMetric = string | SQLMetric | SimpleMetric;
 
 // Type of Metric the client provides to server after unifying various forms
 // of metrics in form data
-export type Metric = {
-  label: string;
-} & Partial<AdhocMetric>;
+export type Metric = BuiltInMetric | SQLMetric | SimpleMetric;
 
 export class Metrics {
   // Use Array to maintain insertion order for metrics that are order sensitive
@@ -84,6 +87,7 @@ export class Metrics {
   private addMetric(metric: FormDataMetric) {
     if (typeof metric === 'string') {
       this.metrics.push({
+        expressionType: ExpressionType.BUILTIN,
         label: metric,
       });
     } else {
@@ -98,7 +102,7 @@ export class Metrics {
     }
   }
 
-  private getDefaultLabel(metric: AdhocMetric) {
+  private getDefaultLabel(metric: SQLMetric | SimpleMetric) {
     let label: string;
     if (metric.expressionType === ExpressionType.SIMPLE) {
       label = `${metric.aggregate}(${metric.column.columnName})`;

diff --git a/...nd/temporary_superset_ui/superset-ui/packages/superset-ui-chart/test/query/Metric.test.ts b/...nd/temporary_superset_ui/superset-ui/packages/superset-ui-chart/test/query/Metric.test.ts
@@ -1,6 +1,6 @@
 import { ColumnType } from '../../src/query/Column';
 import {
-  AdhocMetric,
+  FormDataMetric,
   Aggregate,
   ExpressionType,
   LABEL_MAX_LENGTH,
@@ -20,12 +20,17 @@ describe('Metrics', () => {
       ...formData,
       metric: 'sum__num',
     });
-    expect(metrics.getMetrics()).toEqual([{ label: 'sum__num' }]);
+    expect(metrics.getMetrics()).toEqual([
+      {
+        label: 'sum__num',
+        expressionType: 'BUILTIN',
+      },
+    ]);
     expect(metrics.getLabels()).toEqual(['sum__num']);
   });
 
   it('should build metrics for simple adhoc metrics', () => {
-    const adhocMetric: AdhocMetric = {
+    const adhocMetric: FormDataMetric = {
       aggregate: Aggregate.AVG,
       column: {
         columnName: 'sum_girls',
@@ -54,7 +59,7 @@ describe('Metrics', () => {
   });
 
   it('should build metrics for SQL adhoc metrics', () => {
-    const adhocMetric: AdhocMetric = {
+    const adhocMetric: FormDataMetric = {
       expressionType: ExpressionType.SQL,
       sqlExpression: 'COUNT(sum_girls)',
     };
@@ -73,7 +78,7 @@ describe('Metrics', () => {
   });
 
   it('should build metrics for adhoc metrics with custom labels', () => {
-    const adhocMetric: AdhocMetric = {
+    const adhocMetric: FormDataMetric = {
       expressionType: ExpressionType.SQL,
       label: 'foo',
       sqlExpression: 'COUNT(sum_girls)',
@@ -93,7 +98,7 @@ describe('Metrics', () => {
   });
 
   it('should truncate labels if they are too long', () => {
-    const adhocMetric: AdhocMetric = {
+    const adhocMetric: FormDataMetric = {
       expressionType: ExpressionType.SQL,
       sqlExpression: 'COUNT(verrrrrrrrry_loooooooooooooooooooooong_string)',
     };
@@ -109,7 +114,12 @@ describe('Metrics', () => {
       ...formData,
       metrics: ['sum__num'],
     });
-    expect(metrics.getMetrics()).toEqual([{ label: 'sum__num' }]);
+    expect(metrics.getMetrics()).toEqual([
+      {
+        label: 'sum__num',
+        expressionType: 'BUILTIN',
+      },
+    ]);
     expect(metrics.getLabels()).toEqual(['sum__num']);
   });
 });
diff --git a/...ry_superset_ui/superset-ui/packages/superset-ui-chart/test/query/buildQueryObject.test.ts b/...ry_superset_ui/superset-ui/packages/superset-ui-chart/test/query/buildQueryObject.test.ts
@@ -28,6 +28,11 @@ describe('queryObjectBuilder', () => {
       viz_type: 'table',
       metric: 'sum__num',
     });
-    expect(query.metrics).toEqual([{ label: 'sum__num' }]);
+    expect(query.metrics).toEqual([
+      {
+        label: 'sum__num',
+        expressionType: 'BUILTIN',
+      },
+    ]);
   });
 });