From 623f02bd8432d82d8eb0b53e28718d684e0aa716 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 7 Feb 2023 11:04:30 -0500 Subject: [PATCH 1/9] fix: Time Column on Generic X-axis --- .../src/query/buildQueryContext.ts | 17 +---- .../src/explore/components/SaveModal.tsx | 3 +- superset/common/query_context_factory.py | 62 ++++++++++++++++++- 3 files changed, 63 insertions(+), 19 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts b/superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts index e66c1e201e86..c1dc5bd89d2d 100644 --- a/superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts +++ b/superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts @@ -20,16 +20,11 @@ import buildQueryObject from './buildQueryObject'; import DatasourceKey from './DatasourceKey'; import { QueryFieldAliases, QueryFormData } from './types/QueryFormData'; -import { - BinaryQueryObjectFilterClause, - QueryContext, - QueryObject, -} from './types/Query'; +import { QueryContext, QueryObject } from './types/Query'; import { SetDataMaskHook } from '../chart'; import { JsonObject } from '../connection'; import { normalizeTimeColumn } from './normalizeTimeColumn'; -import { hasGenericChartAxes, isXAxisSet } from './getXAxis'; -import { ensureIsArray } from '../utils'; +import { isXAxisSet } from './getXAxis'; const WRAP_IN_ARRAY = (baseQueryObject: QueryObject) => [baseQueryObject]; @@ -60,14 +55,6 @@ export default function buildQueryContext( // 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)); diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 8552eff7fad3..d62cb4d898c6 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -198,7 +198,8 @@ class SaveModal extends React.Component { ); } - const { url_params, ...formData } = this.props.form_data || {}; + const formData = this.props.form_data || {}; + delete formData.url_params; let dashboard: DashboardGetResponse | null = null; if (this.state.newDashboardName || this.state.saveToDashboardId) { diff --git a/superset/common/query_context_factory.py b/superset/common/query_context_factory.py index 360ee449a413..f6aa273e752c 100644 --- a/superset/common/query_context_factory.py +++ b/superset/common/query_context_factory.py @@ -22,9 +22,11 @@ from superset.charts.dao import ChartDAO from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType from superset.common.query_context import QueryContext +from superset.common.query_object import QueryObject from superset.common.query_object_factory import QueryObjectFactory from superset.datasource.dao import DatasourceDAO from superset.models.slice import Slice +from superset.superset_typing import AdhocColumn from superset.utils.core import DatasourceDict, DatasourceType if TYPE_CHECKING: @@ -65,8 +67,12 @@ def create( result_type = result_type or ChartDataResultType.FULL result_format = result_format or ChartDataResultFormat.JSON queries_ = [ - self._query_object_factory.create( - result_type, datasource=datasource, **query_obj + self._process_query_object( + datasource_model_instance, + form_data, + self._query_object_factory.create( + result_type, datasource=datasource, **query_obj + ), ) for query_obj in queries ] @@ -90,7 +96,6 @@ def create( # pylint: disable=no-self-use def _convert_to_model(self, datasource: DatasourceDict) -> BaseDatasource: - return DatasourceDAO.get_datasource( session=db.session, datasource_type=DatasourceType(datasource["type"]), @@ -99,3 +104,54 @@ def _convert_to_model(self, datasource: DatasourceDict) -> BaseDatasource: def _get_slice(self, slice_id: Any) -> Optional[Slice]: return ChartDAO.find_by_id(slice_id) + + def _process_query_object( + self, + datasource: BaseDatasource, + form_data: Dict[str, Any], + query_object: QueryObject, + ) -> QueryObject: + self._apply_granularity(query_object, form_data, datasource) + self._apply_filters(query_object) + return query_object + + def _apply_granularity( + self, + query_object: QueryObject, + form_data: Dict[str, Any], + datasource: BaseDatasource, + ): + temporal_columns = { + column.column_name for column in datasource.columns if column.is_temporal + } + granularity = query_object.granularity + x_axis = form_data.get("x_axis") + if granularity and x_axis in temporal_columns: + x_axis_column = next( + ( + column + for column in query_object.columns + if column["sqlExpression"] == x_axis + ), + None, + ) + if x_axis_column: + x_axis_column["sqlExpression"] = granularity + x_axis_column["label"] = granularity + query_object.granularity = None + for post_processing in query_object.post_processing: + if post_processing.get("operation") == "pivot": + post_processing["options"]["index"] = [granularity] + + x_axis_filter = x_axis and next( + (filter for filter in query_object.filter if filter["col"] == x_axis), + None, + ) + if x_axis_filter: + x_axis_filter["col"] = granularity + + def _apply_filters(self, query_object: QueryObject): + if query_object.time_range: + for filter in query_object.filter: + if filter["op"] == "TEMPORAL_RANGE": + filter["val"] = query_object.time_range From 837ebf3d14041d17beba26fc3e59b18b1c07acbf Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 7 Feb 2023 11:51:33 -0500 Subject: [PATCH 2/9] Keeps granularity in query_object --- superset/common/query_context_factory.py | 64 +++++++++++++++--------- 1 file changed, 41 insertions(+), 23 deletions(-) diff --git a/superset/common/query_context_factory.py b/superset/common/query_context_factory.py index f6aa273e752c..57316ec0c747 100644 --- a/superset/common/query_context_factory.py +++ b/superset/common/query_context_factory.py @@ -126,29 +126,47 @@ def _apply_granularity( } granularity = query_object.granularity x_axis = form_data.get("x_axis") - if granularity and x_axis in temporal_columns: - x_axis_column = next( - ( - column - for column in query_object.columns - if column["sqlExpression"] == x_axis - ), - None, - ) - if x_axis_column: - x_axis_column["sqlExpression"] = granularity - x_axis_column["label"] = granularity - query_object.granularity = None - for post_processing in query_object.post_processing: - if post_processing.get("operation") == "pivot": - post_processing["options"]["index"] = [granularity] - - x_axis_filter = x_axis and next( - (filter for filter in query_object.filter if filter["col"] == x_axis), - None, - ) - if x_axis_filter: - x_axis_filter["col"] = granularity + + if granularity: + filter_to_remove = None + if x_axis in temporal_columns: + filter_to_remove = x_axis + x_axis_column = next( + ( + column + for column in query_object.columns + if column["sqlExpression"] == x_axis + ), + None, + ) + # Replaces x-axis column values with granularity + if x_axis_column: + x_axis_column["sqlExpression"] = granularity + x_axis_column["label"] = granularity + for post_processing in query_object.post_processing: + if post_processing.get("operation") == "pivot": + post_processing["options"]["index"] = [granularity] + + # If no temporal x-axis, then get the first temporal filter + if not filter_to_remove: + filter_to_remove = next( + ( + filter + for filter in query_object.filter + if filter["op"] == "TEMPORAL_RANGE" + ), + None, + ) + + # Removes the temporal filter which may be an x-axis or the first temporal filter. + # A new filter based on the value of the granularity will be added later in the code. + # In practice, this is replacing the previous default temporal filter. + if filter_to_remove: + query_object.filter = [ + filter + for filter in query_object.filter + if filter["col"] != filter_to_remove["col"] + ] def _apply_filters(self, query_object: QueryObject): if query_object.time_range: From 463e8bf5c68eccf5d263051d6b0380787f907946 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 7 Feb 2023 13:54:49 -0500 Subject: [PATCH 3/9] Fixes filter condition --- superset/common/query_context_factory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/common/query_context_factory.py b/superset/common/query_context_factory.py index 57316ec0c747..a807ebe1e2aa 100644 --- a/superset/common/query_context_factory.py +++ b/superset/common/query_context_factory.py @@ -151,7 +151,7 @@ def _apply_granularity( if not filter_to_remove: filter_to_remove = next( ( - filter + filter["col"] for filter in query_object.filter if filter["op"] == "TEMPORAL_RANGE" ), @@ -165,7 +165,7 @@ def _apply_granularity( query_object.filter = [ filter for filter in query_object.filter - if filter["col"] != filter_to_remove["col"] + if filter["col"] != filter_to_remove ] def _apply_filters(self, query_object: QueryObject): From aad99962ff1e66ed3c25e5617b76455b616c7cd3 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 7 Feb 2023 15:03:33 -0500 Subject: [PATCH 4/9] Checks if granularity is already in the filters --- superset/common/query_context_factory.py | 26 ++++++++++++++---------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/superset/common/query_context_factory.py b/superset/common/query_context_factory.py index a807ebe1e2aa..c2ed68f01f70 100644 --- a/superset/common/query_context_factory.py +++ b/superset/common/query_context_factory.py @@ -147,18 +147,22 @@ def _apply_granularity( if post_processing.get("operation") == "pivot": post_processing["options"]["index"] = [granularity] - # If no temporal x-axis, then get the first temporal filter + # If no temporal x-axis, then get the default temporal filter if not filter_to_remove: - filter_to_remove = next( - ( - filter["col"] - for filter in query_object.filter - if filter["op"] == "TEMPORAL_RANGE" - ), - None, - ) - - # Removes the temporal filter which may be an x-axis or the first temporal filter. + temporal_filters = [ + filter["col"] + for filter in query_object.filter + if filter["op"] == "TEMPORAL_RANGE" + ] + if len(temporal_filters) > 0: + # Use granularity if it's already in the filters + if granularity in temporal_filters: + filter_to_remove = granularity + else: + # Use the first temporal filter + filter_to_remove = temporal_filters[0] + + # Removes the temporal filter which may be an x-axis or another temporal filter. # A new filter based on the value of the granularity will be added later in the code. # In practice, this is replacing the previous default temporal filter. if filter_to_remove: From 1d670ba4fec58c77867fcabacf4aff45347451e3 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 7 Feb 2023 15:27:37 -0500 Subject: [PATCH 5/9] Fixes pylint errors --- superset/common/query_context_factory.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/superset/common/query_context_factory.py b/superset/common/query_context_factory.py index c2ed68f01f70..67a50aaaa1e5 100644 --- a/superset/common/query_context_factory.py +++ b/superset/common/query_context_factory.py @@ -26,7 +26,6 @@ from superset.common.query_object_factory import QueryObjectFactory from superset.datasource.dao import DatasourceDAO from superset.models.slice import Slice -from superset.superset_typing import AdhocColumn from superset.utils.core import DatasourceDict, DatasourceType if TYPE_CHECKING: @@ -162,8 +161,9 @@ def _apply_granularity( # Use the first temporal filter filter_to_remove = temporal_filters[0] - # Removes the temporal filter which may be an x-axis or another temporal filter. - # A new filter based on the value of the granularity will be added later in the code. + # Removes the temporal filter which may be an x-axis or + # another temporal filter. A new filter based on the value of + # the granularity will be added later in the code. # In practice, this is replacing the previous default temporal filter. if filter_to_remove: query_object.filter = [ @@ -174,6 +174,6 @@ def _apply_granularity( def _apply_filters(self, query_object: QueryObject): if query_object.time_range: - for filter in query_object.filter: - if filter["op"] == "TEMPORAL_RANGE": - filter["val"] = query_object.time_range + for filter_object in query_object.filter: + if filter_object["op"] == "TEMPORAL_RANGE": + filter_object["val"] = query_object.time_range From 20a3e45248d23595c45cd54ee55d125e15df548a Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 7 Feb 2023 16:14:41 -0500 Subject: [PATCH 6/9] Fixes mypy errors --- superset/common/query_context_factory.py | 26 ++++++++++++++++-------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/superset/common/query_context_factory.py b/superset/common/query_context_factory.py index 67a50aaaa1e5..e87bc1bbd257 100644 --- a/superset/common/query_context_factory.py +++ b/superset/common/query_context_factory.py @@ -26,6 +26,7 @@ from superset.common.query_object_factory import QueryObjectFactory from superset.datasource.dao import DatasourceDAO from superset.models.slice import Slice +from superset.superset_typing import AdhocColumn from superset.utils.core import DatasourceDict, DatasourceType if TYPE_CHECKING: @@ -107,7 +108,7 @@ def _get_slice(self, slice_id: Any) -> Optional[Slice]: def _process_query_object( self, datasource: BaseDatasource, - form_data: Dict[str, Any], + form_data: Optional[Dict[str, Any]], query_object: QueryObject, ) -> QueryObject: self._apply_granularity(query_object, form_data, datasource) @@ -117,31 +118,38 @@ def _process_query_object( def _apply_granularity( self, query_object: QueryObject, - form_data: Dict[str, Any], + form_data: Optional[Dict[str, Any]], datasource: BaseDatasource, - ): + ) -> None: temporal_columns = { column.column_name for column in datasource.columns if column.is_temporal } granularity = query_object.granularity - x_axis = form_data.get("x_axis") + x_axis = form_data and form_data.get("x_axis") if granularity: filter_to_remove = None - if x_axis in temporal_columns: + if x_axis and x_axis in temporal_columns: filter_to_remove = x_axis x_axis_column = next( ( column for column in query_object.columns - if column["sqlExpression"] == x_axis + if column == x_axis + or (type(column) is dict and column["sqlExpression"] == x_axis) ), None, ) # Replaces x-axis column values with granularity if x_axis_column: - x_axis_column["sqlExpression"] = granularity - x_axis_column["label"] = granularity + if type(x_axis_column) is dict: + x_axis_column["sqlExpression"] = granularity + x_axis_column["label"] = granularity + else: + query_object.columns = [ + granularity if column == x_axis_column else column + for column in query_object.columns + ] for post_processing in query_object.post_processing: if post_processing.get("operation") == "pivot": post_processing["options"]["index"] = [granularity] @@ -172,7 +180,7 @@ def _apply_granularity( if filter["col"] != filter_to_remove ] - def _apply_filters(self, query_object: QueryObject): + def _apply_filters(self, query_object: QueryObject) -> None: if query_object.time_range: for filter_object in query_object.filter: if filter_object["op"] == "TEMPORAL_RANGE": From 42359430558b25e310906273a3e0cc2c4075efc7 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 7 Feb 2023 16:27:38 -0500 Subject: [PATCH 7/9] Removes unused import --- superset/common/query_context_factory.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/superset/common/query_context_factory.py b/superset/common/query_context_factory.py index e87bc1bbd257..286c9c3ec554 100644 --- a/superset/common/query_context_factory.py +++ b/superset/common/query_context_factory.py @@ -26,7 +26,6 @@ from superset.common.query_object_factory import QueryObjectFactory from superset.datasource.dao import DatasourceDAO from superset.models.slice import Slice -from superset.superset_typing import AdhocColumn from superset.utils.core import DatasourceDict, DatasourceType if TYPE_CHECKING: @@ -136,13 +135,16 @@ def _apply_granularity( column for column in query_object.columns if column == x_axis - or (type(column) is dict and column["sqlExpression"] == x_axis) + or ( + isinstance(column, dict) + and column["sqlExpression"] == x_axis + ) ), None, ) # Replaces x-axis column values with granularity if x_axis_column: - if type(x_axis_column) is dict: + if isinstance(x_axis_column, dict): x_axis_column["sqlExpression"] = granularity x_axis_column["label"] = granularity else: From a4e00d033d26e26201bb9a745bbb855a85c8f668 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Wed, 8 Feb 2023 08:45:04 -0500 Subject: [PATCH 8/9] Removes no longer valid test --- .../test/query/buildQueryContext.test.ts | 37 ------------------- 1 file changed, 37 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts b/superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts index e14648a984f9..9d47361e8fdd 100644 --- a/superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts @@ -164,41 +164,4 @@ describe('buildQueryContext', () => { expect(spyNormalizeTimeColumn).not.toBeCalled(); 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'], - }, - ]); - }); }); From 0703e8763d723b63a024f9410deb3a8f2531e73b Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Wed, 8 Feb 2023 11:19:00 -0500 Subject: [PATCH 9/9] Checks if temporal using is_dttm --- superset/common/query_context_factory.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/superset/common/query_context_factory.py b/superset/common/query_context_factory.py index 286c9c3ec554..3a8380bdb0c8 100644 --- a/superset/common/query_context_factory.py +++ b/superset/common/query_context_factory.py @@ -121,7 +121,9 @@ def _apply_granularity( datasource: BaseDatasource, ) -> None: temporal_columns = { - column.column_name for column in datasource.columns if column.is_temporal + column.column_name + for column in datasource.columns + if (column["is_dttm"] if isinstance(column, dict) else column.is_dttm) } granularity = query_object.granularity x_axis = form_data and form_data.get("x_axis")