From 115ed7ae7f71d1130de6f9f29eb36269d824aad9 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 27 Mar 2023 09:48:21 +0300 Subject: [PATCH 1/3] perf(sqla): avoid unnecessary type check on adhoc column --- superset/connectors/sqla/models.py | 32 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index b76e423caf441..8e146f8854881 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1027,28 +1027,28 @@ def adhoc_column_to_sqla( template_processor=template_processor, ) col_in_metadata = self.get_column(expression) + time_grain = col.get("timeGrain") + has_timegrain = col.get("columnType") == "BASE_AXIS" and time_grain + is_dttm = False if col_in_metadata: sqla_column = col_in_metadata.get_sqla_col( template_processor=template_processor ) is_dttm = col_in_metadata.is_temporal else: - try: - sqla_column = literal_column(expression) - # probe adhoc column type - tbl, _ = self.get_from_clause(template_processor) - qry = sa.select([sqla_column]).limit(1).select_from(tbl) - sql = self.database.compile_sqla_query(qry) - col_desc = get_columns_description(self.database, sql) - is_dttm = col_desc[0]["is_dttm"] - except SupersetGenericDBErrorException as ex: - raise ColumnNotFoundException(message=str(ex)) from ex - - if ( - is_dttm - and col.get("columnType") == "BASE_AXIS" - and (time_grain := col.get("timeGrain")) - ): + sqla_column = literal_column(expression) + if has_timegrain: + try: + # probe adhoc column type + tbl, _ = self.get_from_clause(template_processor) + qry = sa.select([sqla_column]).limit(1).select_from(tbl) + sql = self.database.compile_sqla_query(qry) + col_desc = get_columns_description(self.database, sql) + is_dttm = col_desc[0]["is_dttm"] + except SupersetGenericDBErrorException as ex: + raise ColumnNotFoundException(message=str(ex)) from ex + + if is_dttm and has_timegrain: sqla_column = self.db_engine_spec.get_timestamp_expr( col=sqla_column, pdf=None, From de0c2bf42125c96c655b83b4ef043bc9b5b9ff35 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Fri, 31 Mar 2023 14:25:30 +0300 Subject: [PATCH 2/3] add arg to check expression --- superset/connectors/sqla/models.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 8e146f8854881..8289d2924fe10 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1009,12 +1009,16 @@ def adhoc_metric_to_sqla( def adhoc_column_to_sqla( self, col: AdhocColumn, + force_check_expression: bool = False, template_processor: Optional[BaseTemplateProcessor] = None, ) -> ColumnElement: """ Turn an adhoc column into a sqlalchemy column. :param col: Adhoc column definition + :param force_check_expression: Should the expression be checked in the db. + This is needed to validate if a filter with an adhoc column + is applicable. :param template_processor: template_processor instance :returns: The metric defined as a sqlalchemy column :rtype: sqlalchemy.sql.column @@ -1037,7 +1041,7 @@ def adhoc_column_to_sqla( is_dttm = col_in_metadata.is_temporal else: sqla_column = literal_column(expression) - if has_timegrain: + if has_timegrain or force_check_expression: try: # probe adhoc column type tbl, _ = self.get_from_clause(template_processor) @@ -1458,7 +1462,11 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma col_obj = dttm_col elif is_adhoc_column(flt_col): try: - sqla_col = self.adhoc_column_to_sqla(flt_col) + sqla_col = self.adhoc_column_to_sqla( + col=flt_col, + force_check_expression=True, + template_processor=template_processor, + ) applied_adhoc_filters_columns.append(flt_col) except ColumnNotFoundException: rejected_adhoc_filters_columns.append(flt_col) From 3a8b4db76e7f0050d04550d7eb140896687acbe2 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Fri, 31 Mar 2023 17:02:53 +0300 Subject: [PATCH 3/3] lint --- superset/connectors/sqla/models.py | 10 +++++----- superset/utils/pandas_postprocessing/pivot.py | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 8289d2924fe10..593c5f853b935 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1006,17 +1006,17 @@ def adhoc_metric_to_sqla( return self.make_sqla_column_compatible(sqla_metric, label) - def adhoc_column_to_sqla( + def adhoc_column_to_sqla( # pylint: disable=too-many-locals self, col: AdhocColumn, - force_check_expression: bool = False, + force_type_check: bool = False, template_processor: Optional[BaseTemplateProcessor] = None, ) -> ColumnElement: """ Turn an adhoc column into a sqlalchemy column. :param col: Adhoc column definition - :param force_check_expression: Should the expression be checked in the db. + :param force_type_check: Should the column type be checked in the db. This is needed to validate if a filter with an adhoc column is applicable. :param template_processor: template_processor instance @@ -1041,7 +1041,7 @@ def adhoc_column_to_sqla( is_dttm = col_in_metadata.is_temporal else: sqla_column = literal_column(expression) - if has_timegrain or force_check_expression: + if has_timegrain or force_type_check: try: # probe adhoc column type tbl, _ = self.get_from_clause(template_processor) @@ -1464,7 +1464,7 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma try: sqla_col = self.adhoc_column_to_sqla( col=flt_col, - force_check_expression=True, + force_type_check=True, template_processor=template_processor, ) applied_adhoc_filters_columns.append(flt_col) diff --git a/superset/utils/pandas_postprocessing/pivot.py b/superset/utils/pandas_postprocessing/pivot.py index 4083847fffcb5..df5fa7e37cf94 100644 --- a/superset/utils/pandas_postprocessing/pivot.py +++ b/superset/utils/pandas_postprocessing/pivot.py @@ -28,7 +28,7 @@ @validate_column_args("index", "columns") -def pivot( # pylint: disable=too-many-arguments,too-many-locals +def pivot( # pylint: disable=too-many-arguments df: DataFrame, index: List[str], aggregates: Dict[str, Dict[str, Any]],