Skip to content

Commit

Permalink
chore(superset-ui): remove deprecated fields from QueryObject (#22272)
Browse files Browse the repository at this point in the history
  • Loading branch information
villebro authored Nov 30, 2022
1 parent 91d1905 commit b1f8fd4
Show file tree
Hide file tree
Showing 13 changed files with 79 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,18 @@ import {
AdhocFilter,
QueryObject,
QueryObjectFilterClause,
isPhysicalColumn,
isAdhocColumn,
isQueryFormMetric,
} from './types';
import {
QueryFieldAliases,
QueryFormColumn,
QueryFormMetric,
QueryFormData,
} from './types/QueryFormData';
import processFilters from './processFilters';
import extractExtras from './extractExtras';
import extractQueryFields from './extractQueryFields';
import { overrideExtraFormData } from './processExtraFormData';
import { isDefined } from '../utils';

/**
* Build the common segments of all query objects (e.g. the granularity field derived from
Expand Down Expand Up @@ -94,9 +94,9 @@ export default function buildQueryObject<T extends QueryFormData>(
...extras,
...filterFormData,
});
const normalizeSeriesLimitMetric = (column: QueryFormColumn | undefined) => {
if (isAdhocColumn(column) || isPhysicalColumn(column)) {
return column;
const normalizeSeriesLimitMetric = (metric: QueryFormMetric | undefined) => {
if (isQueryFormMetric(metric)) {
return metric;
}
return undefined;
};
Expand All @@ -123,10 +123,11 @@ export default function buildQueryObject<T extends QueryFormData>(
? undefined
: numericRowOffset,
series_columns,
series_limit,
series_limit_metric: normalizeSeriesLimitMetric(series_limit_metric),
timeseries_limit: limit ? Number(limit) : 0,
timeseries_limit_metric: timeseries_limit_metric || undefined,
series_limit: series_limit ?? (isDefined(limit) ? Number(limit) : 0),
series_limit_metric:
normalizeSeriesLimitMetric(series_limit_metric) ??
timeseries_limit_metric ??
undefined,
order_desc: typeof order_desc === 'undefined' ? true : order_desc,
url_params: url_params || undefined,
custom_params,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,20 @@ export default function normalizeOrderBy(

// ensure that remove invalid orderby clause
const cloneQueryObject = { ...queryObject };
delete cloneQueryObject.timeseries_limit_metric;
delete cloneQueryObject.series_limit_metric;
delete cloneQueryObject.legacy_order_by;
delete cloneQueryObject.order_desc;
delete cloneQueryObject.orderby;

const isAsc = !queryObject.order_desc;
if (
queryObject.timeseries_limit_metric !== undefined &&
queryObject.timeseries_limit_metric !== null &&
!isEmpty(queryObject.timeseries_limit_metric)
queryObject.series_limit_metric !== undefined &&
queryObject.series_limit_metric !== null &&
!isEmpty(queryObject.series_limit_metric)
) {
return {
...cloneQueryObject,
orderby: [[queryObject.timeseries_limit_metric, isAsc]],
orderby: [[queryObject.series_limit_metric, isAsc]],
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,6 @@ export interface QueryObject
/** The size of bucket by which to group timeseries data (forthcoming) */
time_grain?: string;

/** Maximum number of timeseries */
timeseries_limit?: number;

/** The metric used to sort the returned result. */
timeseries_limit_metric?: Maybe<QueryFormMetric>;

/** Direction to ordered by */
order_desc?: boolean;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,13 +166,15 @@ export interface BaseFormData extends TimeRange, FormDataResidual {
extra_form_data?: ExtraFormData;
/** order descending */
order_desc?: boolean;
/** limit number of time series */
/** limit number of time series
* deprecated - use series_limit instead */
limit?: number;
/** limit number of row in the results */
row_limit?: string | number | null;
/** row offset for server side pagination */
row_offset?: string | number | null;
/** The metric used to order timeseries for limiting */
/** The metric used to order timeseries for limiting
* deprecated - use series_limit_metric instead */
timeseries_limit_metric?: QueryFormMetric;
/** Force refresh */
force?: boolean;
Expand All @@ -184,7 +186,7 @@ export interface BaseFormData extends TimeRange, FormDataResidual {
/** limit number of series */
series_columns?: QueryFormColumn[];
series_limit?: number;
series_limit_metric?: QueryFormColumn;
series_limit_metric?: QueryFormMetric;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,15 +119,26 @@ describe('buildQueryObject', () => {
expect(query.metrics).toEqual(['sum__num', 'avg__num']);
});

it('should build limit', () => {
const limit = 2;
it('should build series_limit from legacy control', () => {
const series_limit = 2;
query = buildQueryObject({
datasource: '5__table',
granularity_sqla: 'ds',
viz_type: 'table',
limit,
limit: series_limit,
});
expect(query.timeseries_limit).toEqual(limit);
expect(query.series_limit).toEqual(series_limit);
});

it('should build series_limit', () => {
const series_limit = 2;
query = buildQueryObject({
datasource: '5__table',
granularity_sqla: 'ds',
viz_type: 'table',
series_limit,
});
expect(query.series_limit).toEqual(series_limit);
});

it('should build order_desc', () => {
Expand All @@ -141,15 +152,15 @@ describe('buildQueryObject', () => {
expect(query.order_desc).toEqual(orderDesc);
});

it('should build timeseries_limit_metric', () => {
it('should build series_limit_metric from legacy control', () => {
const metric = 'country';
query = buildQueryObject({
datasource: '5__table',
granularity_sqla: 'ds',
viz_type: 'table',
timeseries_limit_metric: metric,
});
expect(query.timeseries_limit_metric).toEqual(metric);
expect(query.series_limit_metric).toEqual(metric);
});

it('should build series_limit_metric', () => {
Expand Down Expand Up @@ -291,6 +302,26 @@ describe('buildQueryObject', () => {
).toBeUndefined();
});

it('should populate granularity', () => {
const granularity = 'ds';
query = buildQueryObject({
datasource: '5__table',
granularity,
viz_type: 'table',
});
expect(query.granularity).toEqual(granularity);
});

it('should populate granularity from legacy field', () => {
const granularity = 'ds';
query = buildQueryObject({
datasource: '5__table',
granularity_sqla: granularity,
viz_type: 'table',
});
expect(query.granularity).toEqual(granularity);
});

it('should populate custom_params', () => {
const customParams: JsonObject = {
customObject: { id: 137, name: 'C-137' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@ describe('normalizeOrderBy', () => {
expect(normalizeOrderBy(query)).toEqual(query);
});

it('has timeseries_limit_metric in queryObject', () => {
it('has series_limit_metric in queryObject', () => {
const query: QueryObject = {
datasource: '5__table',
viz_type: 'table',
time_range: '1 year ago : 2013',
metrics: ['count(*)'],
timeseries_limit_metric: {
series_limit_metric: {
expressionType: 'SIMPLE',
column: {
id: 1,
Expand All @@ -46,7 +46,7 @@ describe('normalizeOrderBy', () => {
order_desc: true,
};
const expectedQueryObject = normalizeOrderBy(query);
expect(expectedQueryObject).not.toHaveProperty('timeseries_limit_metric');
expect(expectedQueryObject).not.toHaveProperty('series_limit_metric');
expect(expectedQueryObject).not.toHaveProperty('order_desc');
expect(expectedQueryObject).toEqual({
datasource: '5__table',
Expand Down Expand Up @@ -118,7 +118,7 @@ describe('normalizeOrderBy', () => {
order_desc: true,
};
const expectedQueryObject = normalizeOrderBy(query);
expect(expectedQueryObject).not.toHaveProperty('timeseries_limit_metric');
expect(expectedQueryObject).not.toHaveProperty('series_limit_metric');
expect(expectedQueryObject).not.toHaveProperty('order_desc');
expect(expectedQueryObject).toEqual({
datasource: '5__table',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ const adhocMetricSQL = {
sqlExpression: 'count(*)',
};

const savedMetric = 'count(*)';

test('isSavedMetric returns true', () => {
expect(isSavedMetric('count(*)')).toEqual(true);
expect(isSavedMetric(savedMetric)).toEqual(true);
});

test('isSavedMetric returns false', () => {
Expand Down Expand Up @@ -76,7 +78,7 @@ test('isAdhocMetricSQL returns false', () => {
test('isQueryFormMetric returns true', () => {
expect(isQueryFormMetric(adhocMetricSQL)).toEqual(true);
expect(isQueryFormMetric(adhocMetricSimple)).toEqual(true);
expect(isQueryFormMetric('count(*)')).toEqual(true);
expect(isQueryFormMetric(savedMetric)).toEqual(true);
});

test('isQueryFormMetric returns false', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import {
} from '@superset-ui/core';

export default function buildQuery(formData: QueryFormData) {
const { timeseries_limit_metric } = formData;
const sortByMetric = ensureIsArray(timeseries_limit_metric)[0];
const { series_limit_metric } = formData;
const sortByMetric = ensureIsArray(series_limit_metric)[0];

return buildQueryContext(formData, baseQueryObject => {
let { metrics, orderby = [] } = baseQueryObject;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,8 @@ test('should compile query object A', () => {
row_limit: 10,
row_offset: undefined,
series_columns: ['foo'],
series_limit: undefined,
series_limit: 5,
series_limit_metric: undefined,
timeseries_limit: 5,
url_params: {},
custom_params: {},
custom_form_data: {},
Expand Down Expand Up @@ -167,9 +166,8 @@ test('should compile query object B', () => {
row_limit: 100,
row_offset: undefined,
series_columns: [],
series_limit: undefined,
series_limit: 0,
series_limit_metric: undefined,
timeseries_limit: 0,
url_params: {},
custom_params: {},
custom_form_data: {},
Expand Down Expand Up @@ -313,9 +311,8 @@ test('should convert a queryObject with x-axis although FF is disabled', () => {
row_limit: 10,
row_offset: undefined,
series_columns: ['foo'],
series_limit: undefined,
series_limit: 5,
series_limit_metric: undefined,
timeseries_limit: 5,
url_params: {},
custom_params: {},
custom_form_data: {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('Timeseries buildQuery', () => {
});
const [query] = queryContext.queries;
expect(query.metrics).toEqual(['bar', 'baz']);
expect(query.timeseries_limit_metric).toEqual('bar');
expect(query.series_limit_metric).toEqual('bar');
expect(query.order_desc).toEqual(true);
expect(query.orderby).toEqual([['bar', false]]);
});
Expand All @@ -55,7 +55,7 @@ describe('Timeseries buildQuery', () => {
});
const [query] = queryContext.queries;
expect(query.metrics).toEqual(['bar', 'baz']);
expect(query.timeseries_limit_metric).toEqual('bar');
expect(query.series_limit_metric).toEqual('bar');
expect(query.order_desc).toEqual(true);
expect(query.orderby).toEqual([['foo', true]]);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ export default function buildQuery(formData: QueryFormData) {
{
...baseQueryObject,
columns: [],
groupby: [],
metrics: [
{
aggregate: 'MIN',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('Select buildQuery', () => {
const queryContext = buildQuery(formData);
expect(queryContext.queries.length).toEqual(1);
const [query] = queryContext.queries;
expect(query.groupby).toEqual(['my_col']);
expect(query.columns).toEqual(['my_col']);
expect(query.filters).toEqual([]);
expect(query.metrics).toEqual([]);
expect(query.orderby).toEqual([]);
Expand All @@ -55,7 +55,7 @@ describe('Select buildQuery', () => {
});
expect(queryContext.queries.length).toEqual(1);
const [query] = queryContext.queries;
expect(query.groupby).toEqual(['my_col']);
expect(query.columns).toEqual(['my_col']);
expect(query.metrics).toEqual(['my_metric']);
expect(query.orderby).toEqual([['my_metric', false]]);
});
Expand All @@ -68,7 +68,7 @@ describe('Select buildQuery', () => {
});
expect(queryContext.queries.length).toEqual(1);
const [query] = queryContext.queries;
expect(query.groupby).toEqual(['my_col']);
expect(query.columns).toEqual(['my_col']);
expect(query.metrics).toEqual(['my_metric']);
expect(query.orderby).toEqual([['my_metric', true]]);
});
Expand All @@ -80,7 +80,7 @@ describe('Select buildQuery', () => {
});
expect(queryContext.queries.length).toEqual(1);
const [query] = queryContext.queries;
expect(query.groupby).toEqual(['my_col']);
expect(query.columns).toEqual(['my_col']);
expect(query.metrics).toEqual([]);
expect(query.orderby).toEqual([['my_col', true]]);
});
Expand All @@ -92,7 +92,7 @@ describe('Select buildQuery', () => {
});
expect(queryContext.queries.length).toEqual(1);
const [query] = queryContext.queries;
expect(query.groupby).toEqual(['my_col']);
expect(query.columns).toEqual(['my_col']);
expect(query.metrics).toEqual([]);
expect(query.orderby).toEqual([['my_col', false]]);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ const buildQuery: BuildQuery<PluginFilterSelectQueryFormData> = (
const query: QueryObject[] = [
{
...baseQueryObject,
groupby: columns,
columns,
metrics: sortMetric ? [sortMetric] : [],
filters: filters.concat(extraFilters),
orderby:
Expand Down

0 comments on commit b1f8fd4

Please sign in to comment.