From d282b041ed93a11eb4ad2d5d87a3cbb679372b7e Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Wed, 8 Nov 2023 19:22:40 +0000 Subject: [PATCH 1/7] always denorm column value before querying values --- superset/datasource/api.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/superset/datasource/api.py b/superset/datasource/api.py index 0c4338e3496d..c8dc74ccbf52 100644 --- a/superset/datasource/api.py +++ b/superset/datasource/api.py @@ -116,8 +116,14 @@ def get_column_values( row_limit = apply_max_row_limit(app.config["FILTER_SELECT_ROW_LIMIT"]) try: + # always denormalize column name before querying for values + db_engine_spec = datasource.database.db_engine_spec + db_dialect = datasource.database.get_dialect() + denomalized_col_name = db_engine_spec.denormalize_name( + db_dialect, column_name + ) payload = datasource.values_for_column( - column_name=column_name, limit=row_limit + column_name=denomalized_col_name, limit=row_limit ) return self.response(200, result=payload) except NotImplementedError: From b1051a732f39abfee6f90435769d8bfc67288269 Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Wed, 8 Nov 2023 21:23:51 +0000 Subject: [PATCH 2/7] cleanup --- superset/connectors/sqla/models.py | 28 ---------------------------- superset/datasource/api.py | 4 ++++ superset/models/helpers.py | 1 - 3 files changed, 4 insertions(+), 29 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index e366940ff259..6935f132d478 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -793,34 +793,6 @@ def get_fetch_values_predicate( ) ) from ex - def values_for_column(self, column_name: str, limit: int = 10000) -> list[Any]: - """Runs query against sqla to retrieve some - sample values for the given column. - """ - cols = {col.column_name: col for col in self.columns} - target_col = cols[column_name] - tp = self.get_template_processor() - tbl, cte = self.get_from_clause(tp) - - qry = ( - select([target_col.get_sqla_col(template_processor=tp)]) - .select_from(tbl) - .distinct() - ) - if limit: - qry = qry.limit(limit) - - if self.fetch_values_predicate: - qry = qry.where(self.get_fetch_values_predicate(template_processor=tp)) - - with self.database.get_sqla_engine_with_context() as engine: - sql = qry.compile(engine, compile_kwargs={"literal_binds": True}) - sql = self._apply_cte(sql, cte) - sql = self.mutate_query_from_config(sql) - - df = pd.read_sql_query(sql=sql, con=engine) - return df[column_name].to_list() - def mutate_query_from_config(self, sql: str) -> str: """Apply config's SQL_QUERY_MUTATOR diff --git a/superset/datasource/api.py b/superset/datasource/api.py index c8dc74ccbf52..95626c9d5b78 100644 --- a/superset/datasource/api.py +++ b/superset/datasource/api.py @@ -126,6 +126,10 @@ def get_column_values( column_name=denomalized_col_name, limit=row_limit ) return self.response(200, result=payload) + except KeyError: + return self.response( + 400, message=f"Column name {column_name} does not exist" + ) except NotImplementedError: return self.response( 400, diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 83ec9ba37c4f..762593b2d457 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -1365,7 +1365,6 @@ def values_for_column(self, column_name: str, limit: int = 10000) -> list[Any]: sql = qry.compile(engine, compile_kwargs={"literal_binds": True}) sql = self._apply_cte(sql, cte) sql = self.mutate_query_from_config(sql) - df = pd.read_sql_query(sql=sql, con=engine) return df[column_name].to_list() From 29b45fb0c865c572873d8eab726b1a618c32790b Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Thu, 9 Nov 2023 00:44:36 +0000 Subject: [PATCH 3/7] refactor --- superset/models/helpers.py | 43 +++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 762593b2d457..ae1ac9c974b7 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -702,8 +702,8 @@ class ExploreMixin: # pylint: disable=too-many-public-methods } @property - def fetch_value_predicate(self) -> str: - return "fix this!" + def fetch_value_predicate(self) -> Optional[str]: + return None @property def type(self) -> str: @@ -1338,35 +1338,34 @@ def get_time_filter( # pylint: disable=too-many-arguments return and_(*l) def values_for_column(self, column_name: str, limit: int = 10000) -> list[Any]: - """Runs query against sqla to retrieve some - sample values for the given column. - """ - cols = {} - for col in self.columns: - if isinstance(col, dict): - cols[col.get("column_name")] = col - else: - cols[col.column_name] = col - - target_col = cols[column_name] - tp = None # todo(hughhhh): add back self.get_template_processor() + # always denormalize column name before querying for values + db_dialect = self.database.get_dialect() + denomalized_col_name = self.database.db_engine_spec.denormalize_name( + db_dialect, column_name + ) + cols = {col.column_name: col for col in self.columns} + target_col = cols[denomalized_col_name] + tp = self.get_template_processor() tbl, cte = self.get_from_clause(tp) - if isinstance(target_col, dict): - sql_column = sa.column(target_col.get("name")) - else: - sql_column = target_col - - qry = sa.select([sql_column]).select_from(tbl).distinct() + qry = ( + sa.select([target_col.get_sqla_col(template_processor=tp)]) + .select_from(tbl) + .distinct() + ) if limit: qry = qry.limit(limit) - with self.database.get_sqla_engine_with_context() as engine: # type: ignore + if self.fetch_values_predicate: + qry = qry.where(self.get_fetch_values_predicate(template_processor=tp)) + + with self.database.get_sqla_engine_with_context() as engine: sql = qry.compile(engine, compile_kwargs={"literal_binds": True}) sql = self._apply_cte(sql, cte) sql = self.mutate_query_from_config(sql) + df = pd.read_sql_query(sql=sql, con=engine) - return df[column_name].to_list() + return df[denomalized_col_name].to_list() def get_timestamp_expression( self, From 2ee8408538e7f6102189ce81f1efc43a65b31faf Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Thu, 9 Nov 2023 14:41:11 +0000 Subject: [PATCH 4/7] ok --- superset/connectors/base/models.py | 7 ------- superset/datasource/api.py | 8 +------- superset/models/helpers.py | 9 +++++---- 3 files changed, 6 insertions(+), 18 deletions(-) diff --git a/superset/connectors/base/models.py b/superset/connectors/base/models.py index d5386c7a66c3..1fc0fde5751c 100644 --- a/superset/connectors/base/models.py +++ b/superset/connectors/base/models.py @@ -496,13 +496,6 @@ def query(self, query_obj: QueryObjectDict) -> QueryResult: """ raise NotImplementedError() - def values_for_column(self, column_name: str, limit: int = 10000) -> list[Any]: - """Given a column, returns an iterable of distinct values - - This is used to populate the dropdown showing a list of - values in filters in the explore view""" - raise NotImplementedError() - @staticmethod def default_query(qry: Query) -> Query: return qry diff --git a/superset/datasource/api.py b/superset/datasource/api.py index 95626c9d5b78..131d11575572 100644 --- a/superset/datasource/api.py +++ b/superset/datasource/api.py @@ -116,14 +116,8 @@ def get_column_values( row_limit = apply_max_row_limit(app.config["FILTER_SELECT_ROW_LIMIT"]) try: - # always denormalize column name before querying for values - db_engine_spec = datasource.database.db_engine_spec - db_dialect = datasource.database.get_dialect() - denomalized_col_name = db_engine_spec.denormalize_name( - db_dialect, column_name - ) payload = datasource.values_for_column( - column_name=denomalized_col_name, limit=row_limit + column_name=column_name, limit=row_limit ) return self.response(200, result=payload) except KeyError: diff --git a/superset/models/helpers.py b/superset/models/helpers.py index ae1ac9c974b7..264f4c04288d 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -700,6 +700,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods "MIN": sa.func.MIN, "MAX": sa.func.MAX, } + fetch_values_predicate = None @property def fetch_value_predicate(self) -> Optional[str]: @@ -1339,8 +1340,8 @@ def get_time_filter( # pylint: disable=too-many-arguments def values_for_column(self, column_name: str, limit: int = 10000) -> list[Any]: # always denormalize column name before querying for values - db_dialect = self.database.get_dialect() - denomalized_col_name = self.database.db_engine_spec.denormalize_name( + db_dialect = self.database.get_dialect() # type: ignore + denomalized_col_name = self.database.db_engine_spec.denormalize_name( # type: ignore db_dialect, column_name ) cols = {col.column_name: col for col in self.columns} @@ -1359,7 +1360,7 @@ def values_for_column(self, column_name: str, limit: int = 10000) -> list[Any]: if self.fetch_values_predicate: qry = qry.where(self.get_fetch_values_predicate(template_processor=tp)) - with self.database.get_sqla_engine_with_context() as engine: + with self.database.get_sqla_engine_with_context() as engine: # type: ignore sql = qry.compile(engine, compile_kwargs={"literal_binds": True}) sql = self._apply_cte(sql, cte) sql = self.mutate_query_from_config(sql) @@ -1937,7 +1938,7 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma ) having_clause_and += [self.text(having)] - if apply_fetch_values_predicate and self.fetch_values_predicate: # type: ignore + if self.fetch_values_predicate: qry = qry.where( self.get_fetch_values_predicate(template_processor=template_processor) ) From ec13b9084b24d546993cf3405afc62949732508b Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Thu, 9 Nov 2023 14:55:08 +0000 Subject: [PATCH 5/7] refactor get_fetch_values_predicate --- superset/models/helpers.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index a88cddd278f5..e64ee0d6f7bf 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -707,10 +707,6 @@ class ExploreMixin: # pylint: disable=too-many-public-methods } fetch_values_predicate = None - @property - def fetch_value_predicate(self) -> Optional[str]: - return None - @property def type(self) -> str: raise NotImplementedError() @@ -786,17 +782,17 @@ def sql(self) -> str: def columns(self) -> list[Any]: raise NotImplementedError() - def get_fetch_values_predicate( - self, template_processor: Optional[BaseTemplateProcessor] = None - ) -> TextClause: - raise NotImplementedError() - def get_extra_cache_keys(self, query_obj: dict[str, Any]) -> list[Hashable]: raise NotImplementedError() def get_template_processor(self, **kwargs: Any) -> BaseTemplateProcessor: raise NotImplementedError() + def get_fetch_values_predicate( + self, template_processor: Optional[BaseTemplateProcessor] = None + ) -> TextClause: + return self.fetch_values_predicate + def get_sqla_row_level_filters( self, template_processor: BaseTemplateProcessor, From 16825c3d82cf53f1ec7c27f039585c8edefe9a36 Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Mon, 13 Nov 2023 16:07:50 +0000 Subject: [PATCH 6/7] fix linting --- superset/connectors/sqla/models.py | 1 - superset/models/helpers.py | 7 +++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 6935f132d478..510ca54ae8ff 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -46,7 +46,6 @@ inspect, Integer, or_, - select, String, Table, Text, diff --git a/superset/models/helpers.py b/superset/models/helpers.py index e64ee0d6f7bf..50aba1f0b0be 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -789,7 +789,10 @@ def get_template_processor(self, **kwargs: Any) -> BaseTemplateProcessor: raise NotImplementedError() def get_fetch_values_predicate( - self, template_processor: Optional[BaseTemplateProcessor] = None + self, + template_processor: Optional[ + BaseTemplateProcessor + ] = None, # pylint: disable=unused-argument ) -> TextClause: return self.fetch_values_predicate @@ -1416,7 +1419,7 @@ def convert_tbl_column_to_sqla_col( def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-many-branches,too-many-statements self, - apply_fetch_values_predicate: bool = False, + apply_fetch_values_predicate: bool = False, # pylint: disable=unused-argument columns: Optional[list[Column]] = None, extras: Optional[dict[str, Any]] = None, filter: Optional[ # pylint: disable=redefined-builtin From eff8fa31ebe20cc4184d7f52abc9fdfa7ea0cf8a Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Mon, 13 Nov 2023 16:17:22 +0000 Subject: [PATCH 7/7] fix helpers --- superset/models/helpers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 50aba1f0b0be..316a46c10ca7 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -790,7 +790,7 @@ def get_template_processor(self, **kwargs: Any) -> BaseTemplateProcessor: def get_fetch_values_predicate( self, - template_processor: Optional[ + template_processor: Optional[ # pylint: disable=unused-argument BaseTemplateProcessor ] = None, # pylint: disable=unused-argument ) -> TextClause: @@ -1419,7 +1419,7 @@ def convert_tbl_column_to_sqla_col( def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-many-branches,too-many-statements self, - apply_fetch_values_predicate: bool = False, # pylint: disable=unused-argument + apply_fetch_values_predicate: bool = False, columns: Optional[list[Column]] = None, extras: Optional[dict[str, Any]] = None, filter: Optional[ # pylint: disable=redefined-builtin @@ -1940,7 +1940,7 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma ) having_clause_and += [self.text(having)] - if self.fetch_values_predicate: + if apply_fetch_values_predicate and self.fetch_values_predicate: qry = qry.where( self.get_fetch_values_predicate(template_processor=template_processor) )