Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support mulitple temporal filters in AdhocFilter and move the Time Section away #21767

Merged
merged 30 commits into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
2be5fe5
feat: support mulitple datetime filter in AdhocFilter and move away t…
zhaoyongjie Oct 12, 2022
7a66d4c
should pass all CI steps
zhaoyongjie Oct 12, 2022
e852a70
add UT for time_range_utils
zhaoyongjie Oct 17, 2022
1950f63
fix time offset
zhaoyongjie Oct 17, 2022
8508eaf
fix pylint
zhaoyongjie Oct 17, 2022
2afa513
datetime_between => temporal_between
zhaoyongjie Oct 17, 2022
26b27c7
add more UT
zhaoyongjie Oct 18, 2022
602f975
fix mysql failure UT
zhaoyongjie Oct 19, 2022
cdb1774
new style in date picker
zhaoyongjie Oct 20, 2022
2cda470
datepicker modal in adhoc filter
zhaoyongjie Oct 20, 2022
1a0bf44
fix overlap in modal
zhaoyongjie Oct 20, 2022
4ba7b8b
fix evalResponse first open modal
zhaoyongjie Oct 20, 2022
950d468
add header
zhaoyongjie Oct 20, 2022
3a3e0cc
apply global default time filter
zhaoyongjie Oct 21, 2022
76199d0
temporal_between => temporal_range
zhaoyongjie Oct 21, 2022
d501d49
code smell
zhaoyongjie Oct 21, 2022
c54f24f
fix drag&drop and save temporal column
zhaoyongjie Oct 21, 2022
43b702f
fix subject change
zhaoyongjie Oct 21, 2022
e8a7d34
fix adhoc filter title
zhaoyongjie Oct 21, 2022
fbbe603
use isTemporalColumn
zhaoyongjie Oct 21, 2022
f76053c
add ut
zhaoyongjie Oct 21, 2022
3196eed
fix previous UT
zhaoyongjie Oct 21, 2022
7767870
fix ut
zhaoyongjie Oct 22, 2022
cbc1be3
support Handlebars
zhaoyongjie Oct 22, 2022
8a48fd0
ut for time filter
zhaoyongjie Oct 25, 2022
905c60e
addressing comments
zhaoyongjie Oct 28, 2022
958c5da
fix lint
zhaoyongjie Oct 28, 2022
207f9b8
fix ut
zhaoyongjie Oct 28, 2022
a10d259
fix label error when custom SQL => simple panel
zhaoyongjie Oct 31, 2022
059fb2f
ut for useDatePickerInAdhocfilter
zhaoyongjie Oct 31, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
* specific language governing permissions and limitations
* under the License.
*/
import { Dataset } from '@superset-ui/chart-controls';
import { DatasourceType } from '@superset-ui/core';
import { Dataset } from './types';

export const TestDataset: Dataset = {
column_format: {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,4 @@ export * from './shared-controls/emitFilterControl';
export * from './shared-controls/components';
export * from './types';
export * from './shared-controls/mixins';
export * from './fixtures';
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,23 @@ export const legacyTimeseriesTime: ControlPanelSectionConfig = {
],
};

export const genericTime: ControlPanelSectionConfig = {
...baseTimeSection,
controlSetRows: [
['granularity_sqla'],
[hasGenericChartAxes ? null : 'time_grain_sqla'],
['time_range'],
],
};
export const genericTime: ControlPanelSectionConfig = hasGenericChartAxes
? { controlSetRows: [] }
: {
...baseTimeSection,
controlSetRows: [
['granularity_sqla'],
['time_grain_sqla'],
['time_range'],
],
};

export const legacyRegularTime: ControlPanelSectionConfig = {
...baseTimeSection,
controlSetRows: [['granularity_sqla'], ['time_range']],
};
export const legacyRegularTime: ControlPanelSectionConfig = hasGenericChartAxes
? { controlSetRows: [] }
: {
...baseTimeSection,
controlSetRows: [['granularity_sqla'], ['time_range']],
};

export const datasourceAndVizType: ControlPanelSectionConfig = {
label: t('Datasource & Chart Type'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@ import {
ColumnMeta,
FilterOption,
temporalColumnMixin,
datePickerInAdhocFilterMixin,
xAxisMixin,
} from '..';
import { xAxisMixin } from './mixins';

type Control = {
savedMetrics?: Metric[] | null;
Expand Down Expand Up @@ -149,6 +150,7 @@ export const dndAdhocFilterControl: SharedControlConfig<
datasource,
}),
provideFormDataToProps: true,
...datePickerInAdhocFilterMixin,
};

export const dndAdhocMetricsControl: SharedControlConfig<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
* under the License.
*/
import {
ensureIsArray,
hasGenericChartAxes,
NO_TIME_RANGE,
QueryFormData,
t,
validateNonEmpty,
Expand Down Expand Up @@ -63,3 +65,57 @@ export const temporalColumnMixin: Pick<BaseControlConfig, 'mapStateToProps'> = {
};
},
};

export const datePickerInAdhocFilterMixin: Pick<
BaseControlConfig,
'initialValue'
> = {
initialValue: (control: ControlState, state: ControlPanelState | null) => {
// skip initialValue if
// 1) GENERIC_CHART_AXES is disabled
// 2) there was a time filter in adhoc filters
if (
!hasGenericChartAxes ||
ensureIsArray(control.value).findIndex(
(flt: any) => flt?.operator === 'TEMPORAL_RANGE',
) > -1
) {
return undefined;
}

// should migrate original granularity_sqla and time_range into adhoc filter
// 1) granularity_sqla and time_range are existed
if (state?.form_data?.granularity_sqla && state?.form_data?.time_range) {
return [
...ensureIsArray(control.value),
{
clause: 'WHERE',
subject: state.form_data.granularity_sqla,
operator: 'TEMPORAL_RANGE',
comparator: state.form_data.time_range,
expressionType: 'SIMPLE',
},
];
}

// should apply the default time filter into adhoc filter
// 1) temporal column is existed in current datasource
const temporalColumn =
state?.datasource &&
getTemporalColumns(state.datasource).defaultTemporalColumn;
if (hasGenericChartAxes && temporalColumn) {
return [
...ensureIsArray(control.value),
{
clause: 'WHERE',
subject: temporalColumn,
operator: 'TEMPORAL_RANGE',
comparator: state?.common?.conf?.DEFAULT_TIME_FILTER || NO_TIME_RANGE,
expressionType: 'SIMPLE',
},
];
}

return undefined;
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import type {
AdhocColumn,
Column,
DatasourceType,
JsonObject,
JsonValue,
Metric,
QueryColumn,
QueryFormColumn,
QueryFormData,
QueryFormMetric,
Expand Down Expand Up @@ -80,12 +80,15 @@ export interface Dataset {
description: string | null;
uid?: string;
owners?: Owner[];
filter_select?: boolean;
filter_select_enabled?: boolean;
}

export interface ControlPanelState {
form_data: QueryFormData;
datasource: Dataset | QueryResponse | null;
controls: ControlStateMapping;
common: JsonObject;
}

/**
Expand Down Expand Up @@ -449,9 +452,7 @@ export type ColorFormatters = {

export default {};

export function isColumnMeta(
column: AdhocColumn | ColumnMeta | QueryColumn,
): column is ColumnMeta {
export function isColumnMeta(column: AnyDict): column is ColumnMeta {
return !!column && 'column_name' in column;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ import {
isQueryResponse,
} from '@superset-ui/chart-controls';

export const getTemporalColumns = (
export function getTemporalColumns(
Copy link
Member

Choose a reason for hiding this comment

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

slightly unrelated - it would be nice to have an ESLint rule for this to make the JS/TS code more consistent

datasource: ValueOf<Pick<ControlPanelState, 'datasource'>>,
) => {
) {
const rv: {
temporalColumns: ColumnMeta[] | QueryColumn[];
defaultTemporalColumn: string | null | undefined;
Expand Down Expand Up @@ -61,4 +61,17 @@ export const getTemporalColumns = (
}

return rv;
};
}

export function isTemporalColumn(
columnName: string,
datasource: ValueOf<Pick<ControlPanelState, 'datasource'>>,
): boolean {
const columns = getTemporalColumns(datasource).temporalColumns;
for (let i = 0; i < columns.length; i += 1) {
if (columns[i].column_name === columnName) {
return true;
}
}
return false;
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,4 @@ export { default as mainMetric } from './mainMetric';
export { default as columnChoices } from './columnChoices';
export * from './defineSavedMetrics';
export * from './getStandardizedControls';
export { getTemporalColumns } from './getTemporalColumns';
export * from './getTemporalColumns';
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@
* under the License.
*/
import { testQueryResponse, testQueryResults } from '@superset-ui/core';
import { Dataset, getTemporalColumns } from '../../src';
import { TestDataset } from '../fixtures';
import {
Dataset,
getTemporalColumns,
isTemporalColumn,
TestDataset,
} from '../../src';

test('get temporal columns from a Dataset', () => {
expect(getTemporalColumns(TestDataset)).toEqual({
Expand Down Expand Up @@ -93,3 +97,8 @@ test('should accept empty Dataset or queryResponse', () => {
defaultTemporalColumn: undefined,
});
});

test('should determine temporal columns in a Dataset', () => {
expect(isTemporalColumn('ds', TestDataset)).toBeTruthy();
expect(isTemporalColumn('num', TestDataset)).toBeFalsy();
});
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,16 @@
import buildQueryObject from './buildQueryObject';
import DatasourceKey from './DatasourceKey';
import { QueryFieldAliases, QueryFormData } from './types/QueryFormData';
import { QueryContext, QueryObject } from './types/Query';
import {
BinaryQueryObjectFilterClause,
QueryContext,
QueryObject,
} from './types/Query';
import { SetDataMaskHook } from '../chart';
import { JsonObject } from '../connection';
import { normalizeTimeColumn } from './normalizeTimeColumn';
import { isXAxisSet } from './getXAxis';
import { hasGenericChartAxes, isXAxisSet } from './getXAxis';
import { ensureIsArray } from '../utils';

const WRAP_IN_ARRAY = (baseQueryObject: QueryObject) => [baseQueryObject];

Expand All @@ -48,15 +53,26 @@ export default function buildQueryContext(
? { buildQuery: options, queryFields: {} }
: options || {};
let queries = buildQuery(buildQueryObject(formData, queryFields));
// --- query mutator begin ---
// todo(Yongjie): move the query mutator into buildQueryObject instead of buildQueryContext
queries.forEach(query => {
if (Array.isArray(query.post_processing)) {
// eslint-disable-next-line no-param-reassign
query.post_processing = query.post_processing.filter(Boolean);
}
if (hasGenericChartAxes && query.time_range) {
// eslint-disable-next-line no-param-reassign
query.filters = ensureIsArray(query.filters).map(flt =>
flt?.op === 'TEMPORAL_RANGE'
? ({ ...flt, val: query.time_range } as BinaryQueryObjectFilterClause)
: flt,
);
}
});
if (isXAxisSet(formData)) {
queries = queries.map(query => normalizeTimeColumn(formData, query));
}
// --- query mutator end ---
return {
datasource: new DatasourceKey(formData.datasource).toObject(),
force: formData.force || false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const BINARY_OPERATORS = [
'ILIKE',
'LIKE',
'REGEX',
'TEMPORAL_RANGE',
] as const;

/** List of operators that require another operand that is a set */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ export const CtasEnum = {

export type QueryColumn = {
name: string;
column_name?: string;
type: string | null;
is_dttm: boolean;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
import { buildQueryContext } from '@superset-ui/core';
import * as queryModule from '../../src/query/normalizeTimeColumn';
import * as getXAxisModule from '../../src/query/getXAxis';
villebro marked this conversation as resolved.
Show resolved Hide resolved

describe('buildQueryContext', () => {
it('should build datasource for table sources and apply defaults', () => {
Expand Down Expand Up @@ -98,6 +99,7 @@ describe('buildQueryContext', () => {
]),
);
});
// todo(Yongjie): move these test case into buildQueryObject.test.ts
it('should remove undefined value in post_processing', () => {
const queryContext = buildQueryContext(
{
Expand All @@ -124,12 +126,9 @@ describe('buildQueryContext', () => {
]);
});
it('should call normalizeTimeColumn if GENERIC_CHART_AXES is enabled and has x_axis', () => {
// @ts-ignore
const spy = jest.spyOn(window, 'window', 'get').mockImplementation(() => ({
featureFlags: {
GENERIC_CHART_AXES: true,
},
}));
Object.defineProperty(getXAxisModule, 'hasGenericChartAxes', {
value: true,
});
const spyNormalizeTimeColumn = jest.spyOn(
queryModule,
'normalizeTimeColumn',
Expand All @@ -144,16 +143,12 @@ describe('buildQueryContext', () => {
() => [{}],
);
expect(spyNormalizeTimeColumn).toBeCalled();
spy.mockRestore();
spyNormalizeTimeColumn.mockRestore();
});
it("shouldn't call normalizeTimeColumn if GENERIC_CHART_AXES is disabled", () => {
// @ts-ignore
const spy = jest.spyOn(window, 'window', 'get').mockImplementation(() => ({
featureFlags: {
GENERIC_CHART_AXES: false,
},
}));
Object.defineProperty(getXAxisModule, 'hasGenericChartAxes', {
value: false,
});
const spyNormalizeTimeColumn = jest.spyOn(
queryModule,
'normalizeTimeColumn',
Expand All @@ -167,7 +162,43 @@ describe('buildQueryContext', () => {
() => [{}],
);
expect(spyNormalizeTimeColumn).not.toBeCalled();
spy.mockRestore();
spyNormalizeTimeColumn.mockRestore();
});
it('should orverride time filter if GENERIC_CHART_AXES is enabled', () => {
Object.defineProperty(getXAxisModule, 'hasGenericChartAxes', {
value: true,
});

const queryContext = buildQueryContext(
{
datasource: '5__table',
viz_type: 'table',
},
() => [
{
filters: [
{
col: 'col1',
op: 'TEMPORAL_RANGE',
val: '2001 : 2002',
},
{
col: 'col2',
op: 'IN',
val: ['a', 'b'],
},
],
time_range: '1990 : 1991',
},
],
);
expect(queryContext.queries[0].filters).toEqual([
{ col: 'col1', op: 'TEMPORAL_RANGE', val: '1990 : 1991' },
{
col: 'col2',
op: 'IN',
val: ['a', 'b'],
},
]);
});
});
Loading