You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2018/11/30 04:57:57 UTC
[GitHub] williaster closed pull request #6423: [SIP-5] Build metrics in
query_object in the client
williaster closed pull request #6423: [SIP-5] Build metrics in query_object in the client
URL: https://github.com/apache/incubator-superset/pull/6423
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/superset/assets/package.json b/superset/assets/package.json
index 9ee1ed4d1a..900c86be53 100644
--- a/superset/assets/package.json
+++ b/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"
diff --git a/superset/assets/spec/javascripts/superset-ui/Metric.test.ts b/superset/assets/spec/javascripts/superset-ui/Metric.test.ts
new file mode 100644
index 0000000000..b1c1da02d9
--- /dev/null
+++ b/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);
+ });
+});
diff --git a/superset/assets/spec/javascripts/superset-ui/buildQueryObject.test.ts b/superset/assets/spec/javascripts/superset-ui/buildQueryObject.test.ts
index ec111dfdad..f0807d7dbf 100644
--- a/superset/assets/spec/javascripts/superset-ui/buildQueryObject.test.ts
+++ b/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'}]);
+ });
});
diff --git a/superset/assets/src/query/Column.ts b/superset/assets/src/query/Column.ts
new file mode 100644
index 0000000000..4125112477
--- /dev/null
+++ b/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;
+}
diff --git a/superset/assets/src/query/FormData.ts b/superset/assets/src/query/FormData.ts
index daa7abf161..9a16f8e44e 100644
--- a/superset/assets/src/query/FormData.ts
+++ b/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;
diff --git a/superset/assets/src/query/Metric.ts b/superset/assets/src/query/Metric.ts
new file mode 100644
index 0000000000..c5e823536c
--- /dev/null
+++ b/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)}...`;
+ }
+}
diff --git a/superset/assets/src/query/buildQueryObject.ts b/superset/assets/src/query/buildQueryObject.ts
index 578d9ae531..203a214179 100644
--- a/superset/assets/src/query/buildQueryObject.ts
+++ b/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(),
};
}
diff --git a/superset/common/query_object.py b/superset/common/query_object.py
index 2052ff9eef..8116d269c4 100644
--- a/superset/common/query_object.py
+++ b/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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org