Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat(core): add extra form data fields for native filters #992

Merged
merged 3 commits into from
Mar 8, 2021
Merged

feat(core): add extra form data fields for native filters #992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8239476
feat(core): implement time grain and column filters
villebro Mar 2, 2021
555dd3b
add apply_fetch_values_predicate to QueryObject
villebro Mar 5, 2021
bd02652
clean up extract types and fix tests
villebro Mar 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions packages/superset-ui-core/src/query/buildQueryObject.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
/* eslint-disable camelcase */
import { QueryObject } from './types/Query';
import { QueryObject, QueryObjectFilterClause } from './types/Query';
import { QueryFieldAliases, QueryFormData } from './types/QueryFormData';
import processFilters from './processFilters';
import extractExtras from './extractExtras';
import extractQueryFields from './extractQueryFields';
import { appendExtraFormData, overrideExtraFormData } from './processExtraFormData';
import { overrideExtraFormData } from './processExtraFormData';
import { AdhocFilter } from './types';

export const DTTM_ALIAS = '__timestamp';

Expand Down Expand Up @@ -41,10 +42,21 @@ export default function buildQueryObject<T extends QueryFormData>(
const numericRowOffset = Number(row_offset);
const { metrics, columns, orderby } = extractQueryFields(residualFormData, queryFields);

// collect all filters for conversion to simple filters/freeform clauses
const extras = extractExtras(formData);
const { filters: extraFilters } = extras;
const { adhoc_filters: appendAdhocFilters = [], filters: appendFilters = [] } = append_form_data;
const filterFormData: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 questions:

  1. Should this functionality to be in extractExtras?
  2. Should we add here also filters and adhoc_filters from custom_form_data and override_form_data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. extractExtras and processFilters probably need to be slightly refactored now that there's more stuff going on here, but since this code is slightly convoluted I prefer to keep these changes as small as possible for now. Let's do a proper refactor soon when we know the full scope of changes required by native and cross filters.
  2. I don't think we should to allow overriding filters, as it could break charts, but happy to open up this discussion if there are known use cases for it. Also, custom_form_data probably shouldn't include filters and adhoc_filters, as those are already defined in QueryFormData and QueryObject.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About 2. For example what will be behavior for a case:
I have table with server pagination and cross filtering:

  1. User clicks Page 2 we have in custom_form_data data about server pagination
  2. We have button in dashboards like resetDashboard so we need reset it to Page 1 or stuff like that

filters: QueryObjectFilterClause[];
adhoc_filters: AdhocFilter[];
} = {
filters: [...extraFilters, ...appendFilters],
adhoc_filters: [...(formData.adhoc_filters || []), ...appendAdhocFilters],
};
const extrasAndfilters = processFilters({
...formData,
...extras,
...filterFormData,
});

let queryObject: QueryObject = {
Expand All @@ -68,8 +80,8 @@ export default function buildQueryObject<T extends QueryFormData>(
url_params: url_params || undefined,
custom_params,
};
// append and override extra form data used by native filters
queryObject = appendExtraFormData(queryObject, append_form_data);
// override extra form data used by native and cross filters
queryObject = overrideExtraFormData(queryObject, override_form_data);

return { ...queryObject, custom_form_data };
}
52 changes: 36 additions & 16 deletions packages/superset-ui-core/src/query/extractExtras.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,38 @@
/* eslint-disable camelcase */
import { isDruidFormData, QueryFormData } from './types/QueryFormData';
import { QueryObject } from './types/Query';
import { AppliedTimeExtras, TimeColumnConfigKey } from './types/Time';
import {
AppliedTimeExtras,
isDruidFormData,
QueryFormData,
QueryObjectExtras,
QueryObjectFilterClause,
TimeColumnConfigKey,
} from './types';

export default function extractExtras(formData: QueryFormData): Partial<QueryObject> {
type ExtraFilterQueryField = {
time_range?: string;
granularity_sqla?: string;
time_grain_sqla?: string;
druid_time_origin?: string;
granularity?: string;
};

type ExtractedExtra = ExtraFilterQueryField & {
filters: QueryObjectFilterClause[];
extras: QueryObjectExtras;
applied_time_extras: AppliedTimeExtras;
};

export default function extractExtras(formData: QueryFormData): ExtractedExtra {
const applied_time_extras: AppliedTimeExtras = {};
const { extras = {}, filters = [] } = formData;
const partialQueryObject: Partial<QueryObject> = {
const filters: QueryObjectFilterClause[] = [];
const extras: QueryObjectExtras = {};
const extract: ExtractedExtra = {
filters,
extras,
applied_time_extras,
};

const reservedColumnsToQueryField: Record<TimeColumnConfigKey, keyof QueryObject> = {
const reservedColumnsToQueryField: Record<TimeColumnConfigKey, keyof ExtraFilterQueryField> = {
__time_range: 'time_range',
__time_col: 'granularity_sqla',
__time_grain: 'time_grain_sqla',
Expand All @@ -24,28 +44,28 @@ export default function extractExtras(formData: QueryFormData): Partial<QueryObj
if (filter.col in reservedColumnsToQueryField) {
const key = filter.col as TimeColumnConfigKey;
const queryField = reservedColumnsToQueryField[key];
partialQueryObject[queryField] = filter.val;
extract[queryField] = filter.val as string;
applied_time_extras[key] = filter.val as string;
} else {
filters.push(filter);
}
});

// map to undeprecated names and remove deprecated fields
if (isDruidFormData(formData) && !partialQueryObject.druid_time_origin) {
if (isDruidFormData(formData) && !extract.druid_time_origin) {
extras.druid_time_origin = formData.druid_time_origin;
delete partialQueryObject.druid_time_origin;
delete extract.druid_time_origin;
} else {
// SQL
extras.time_grain_sqla = partialQueryObject.time_grain_sqla || formData.time_grain_sqla;
partialQueryObject.granularity =
partialQueryObject.granularity_sqla || formData.granularity || formData.granularity_sqla;
delete partialQueryObject.granularity_sqla;
delete partialQueryObject.time_grain_sqla;
extras.time_grain_sqla = extract.time_grain_sqla || formData.time_grain_sqla;
extract.granularity =
extract.granularity_sqla || formData.granularity || formData.granularity_sqla;
delete extract.granularity_sqla;
delete extract.time_grain_sqla;
}

// map time range endpoints:
if (formData.time_range_endpoints) extras.time_range_endpoints = formData.time_range_endpoints;

return partialQueryObject;
return extract;
}
64 changes: 26 additions & 38 deletions packages/superset-ui-core/src/query/processExtraFormData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,55 +16,43 @@
* specific language governing permissions and limitations
* under the License.
*/
import { QueryObject } from './types/Query';
import { QueryObject, QueryObjectExtras, SqlaFormData } from './types';

const ALLOWED_OVERRIDES = ['time_grain_sqla', 'time_range', 'since', 'until'];
const ALLOWED_APPENDS = ['adhoc_filters', 'filters', 'groupby'];
const ALLOWED_OVERRIDES: Record<keyof SqlaFormData, keyof QueryObject> = {
granularity: 'granularity',
granularity_sqla: 'granularity',
time_column: 'time_column',
time_grain: 'time_grain',
time_range: 'time_range',
};
const ALLOWED_EXTRAS_OVERRIDES: (keyof QueryObjectExtras)[] = [
'druid_time_origin',
'relative_start',
'relative_end',
'time_grain_sqla',
'time_range_endpoints',
];

export function overrideExtraFormData(
queryObject: QueryObject,
overrideFormData: Partial<QueryObject>,
): QueryObject {
const overriddenFormData = { ...queryObject };
ALLOWED_OVERRIDES.forEach(key => {
const overriddenFormData: QueryObject = { ...queryObject };
const { extras: overriddenExtras = {} } = overriddenFormData;
Object.entries(ALLOWED_OVERRIDES).forEach(([key, target]) => {
if (key in overrideFormData) {
overriddenFormData[key] = overrideFormData[key];
overriddenFormData[target] = overrideFormData[key];
}
});
return overriddenFormData;
}

export function appendExtraFormData(
queryObject: QueryObject,
appendFormData: Partial<QueryObject>,
): QueryObject {
const appendedFormData = { ...queryObject };
ALLOWED_APPENDS.forEach(key => {
if (key in appendFormData) {
const append = appendFormData[key];
const currentValue = appendedFormData[key] || [];
const { extras: overrideExtras = {} } = overrideFormData;
ALLOWED_EXTRAS_OVERRIDES.forEach(key => {
if (key in overrideExtras) {
// @ts-ignore
currentValue.push(...append);
appendedFormData[key] = currentValue;
overriddenExtras[key] = overrideExtras[key];
}
});

// Add freeform where
const { extras = {} } = appendedFormData;
const { where = '' } = extras;
extras.where = where;

const { extras: appendExtras = {} } = appendFormData;
let { where: appendWhere } = appendExtras;

if (appendWhere) {
appendedFormData.extras = extras;
appendWhere = `(${appendWhere})`;
}
if (where) {
appendWhere = appendWhere ? `(${where}) AND ${appendWhere}` : where;
if (Object.keys(overriddenExtras).length > 0) {
overriddenFormData.extras = overriddenExtras;
}
extras.where = appendWhere;

return appendedFormData;
return overriddenFormData;
}
2 changes: 1 addition & 1 deletion packages/superset-ui-core/src/query/processFilters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { isSimpleAdhocFilter } from './types/Filter';
import convertFilter from './convertFilter';

/** Logic formerly in viz.py's process_query_filters */
export default function processFilters(formData: QueryFormData): Partial<QueryFormData> {
export default function processFilters(formData: Partial<QueryFormData>): Partial<QueryFormData> {
// Split adhoc_filters into four fields according to
// (1) clause (WHERE or HAVING)
// (2) expressionType
Expand Down
14 changes: 13 additions & 1 deletion packages/superset-ui-core/src/query/types/Query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ export interface QueryObject extends QueryFields, TimeRange, ResidualQueryObject
/** Time filters that have been applied to the query object */
applied_time_extras?: AppliedTimeExtras;

/** add fetch value predicate to query if defined in datasource */
apply_fetch_values_predicate?: boolean;

/**
* Extra form data. Current stores information about time granularity, may be
* cleaned up in the future.
Expand All @@ -85,12 +88,15 @@ export interface QueryObject extends QueryFields, TimeRange, ResidualQueryObject
/** SIMPLE where filters */
filters?: QueryObjectFilterClause[];

/** Time column. */
/** Time column for SQL, time-grain for Druid (deprecated) */
granularity?: string;

/** If set, will group by timestamp */
is_timeseries?: boolean;

/** Should the rowcount of the query be fetched */
is_rowcount?: boolean;

/** Free-form HAVING SQL, multiple clauses are concatenated by AND */
having?: string;

Expand All @@ -105,6 +111,12 @@ export interface QueryObject extends QueryFields, TimeRange, ResidualQueryObject
/** Number of rows to skip */
row_offset?: number;

/** The column to which direct temporal filters (forthcoming) */
time_column?: string;

/** The size of bucket by which to group timeseries data (forthcoming) */
time_grain?: string;

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

Expand Down
35 changes: 35 additions & 0 deletions packages/superset-ui-core/test/query/buildQueryObject.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,41 @@ describe('buildQueryObject', () => {
expect(query.metrics).toEqual(['sum__num', 'avg__num']);
});

it('should merge original and append filters', () => {
query = buildQueryObject({
datasource: '5__table',
granularity_sqla: 'ds',
viz_type: 'table',
extra_filters: [{ col: 'abc', op: '==', val: 'qwerty' }],
adhoc_filters: [
{
expressionType: 'SIMPLE',
clause: 'WHERE',
subject: 'foo',
operator: '!=',
comparator: 'bar',
},
],
where: 'a = b',
extra_form_data: {
append_form_data: {
adhoc_filters: [
{
expressionType: 'SQL',
clause: 'WHERE',
sqlExpression: '(1 = 1)',
},
],
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about adhoc_filters and filters. Is my understanding correct that filters are only used for native filters and can only be set from append_form_data, since all filters defined in chart formData are basically adhoc_filters?

I'm a little confused why do we need to allow appending and overriding both filters and adhoc_filters. The overrides are only from a Dashboard's native/cross filters, which would be only in QueryObjectFilterClause or QueryFormExtraFilter format.

It seems to me the original design of an adhoc_filters vs extra_filters was cleaner.

Not sure if this is the direction of future refactoring, but we probably shouldn't add extra stuff that are not configurable by chart controls to formData. This extra_form_data could be moved to the extra options of buildQueryObject and buildQuery. E.g.

buildQueryObject(formData, {
  queryFields: ...         // configured in chart metatada,
  extraFormData: {
    append: {
      adhoc_filter: [...],
    },
    override: {
    }
  }                         // collected from dashboard state
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK filters are just the simple filters that preceeded adhoc_filters (before my time). I'm not sure why we support both, as adhoc_filters are a superset of filters, but my assumption is it's a leftover from old times. I've been using the filters property when adding filters that are simple, i.e. don't require custom SQL, but we could equally well ditch filters and just make it possible append to adhoc_filters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably:

  1. Rename the Python attribute query_object.filter to query_object.filters
  2. Rename adhoc_filters on the client side to filters and have it support both simple filter and adhoc filters.
  3. Move the filter format consolidation logic to the server side. Either in QueryObject itself or when actually building the SQL queries.

},
});
expect(query.filters).toEqual([
{ col: 'abc', op: '==', val: 'qwerty' },
{ col: 'foo', op: '!=', val: 'bar' },
]);
expect(query.extras?.where).toEqual('(a = b) AND ((1 = 1))');
});

it('should group custom metric control', () => {
query = buildQueryObject(
{
Expand Down
70 changes: 56 additions & 14 deletions packages/superset-ui-core/test/query/extractExtras.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,6 @@ describe('extractExtras', () => {
granularity_sqla: 'ds',
time_grain_sqla: 'PT1M',
viz_type: 'my_viz',
filters: [
{
col: 'gender',
op: '==',
val: 'girl',
},
],
};

it('should populate time range endpoints and override formData with double underscored date options', () => {
Expand Down Expand Up @@ -66,13 +59,7 @@ describe('extractExtras', () => {
time_grain_sqla: 'PT5M',
time_range_endpoints: ['inclusive', 'exclusive'],
},
filters: [
{
col: 'gender',
op: '==',
val: 'girl',
},
],
filters: [],
granularity: 'ds2',
time_range: '2009-07-17T00:00:00 : 2020-07-17T00:00:00',
});
Expand All @@ -83,6 +70,11 @@ describe('extractExtras', () => {
extractExtras({
...baseQueryFormData,
extra_filters: [
{
col: 'gender',
op: '==',
val: 'girl',
},
{
col: 'name',
op: 'IN',
Expand Down Expand Up @@ -110,4 +102,54 @@ describe('extractExtras', () => {
granularity: 'ds',
});
});

it('should create regular filters from reserved and non-reserved columns', () => {
expect(
extractExtras({
...baseQueryFormData,
time_range_endpoints: ['inclusive', 'exclusive'],
extra_filters: [
{
col: 'gender',
op: '==',
val: 'girl',
},
{
col: '__time_col',
op: '==',
val: 'ds2',
},
{
col: '__time_grain',
op: '==',
val: 'PT5M',
},
{
col: '__time_range',
op: '==',
val: '2009-07-17T00:00:00 : 2020-07-17T00:00:00',
},
],
}),
).toEqual({
applied_time_extras: {
__time_col: 'ds2',
__time_grain: 'PT5M',
__time_range: '2009-07-17T00:00:00 : 2020-07-17T00:00:00',
},
extras: {
time_grain_sqla: 'PT5M',
time_range_endpoints: ['inclusive', 'exclusive'],
},
filters: [
{
col: 'gender',
op: '==',
val: 'girl',
},
],
granularity: 'ds2',
time_range: '2009-07-17T00:00:00 : 2020-07-17T00:00:00',
});
});
});
Loading