Copilot commented on code in PR #36034:
URL: https://github.com/apache/superset/pull/36034#discussion_r2504921075
##########
superset/models/helpers.py:
##########
@@ -974,7 +974,37 @@ def get_query_str_extended(
query_obj: QueryObjectDict,
mutate: bool = True,
) -> QueryStringExtended:
- sqlaq = self.get_sqla_query(**query_obj)
+ # Filter out keys that aren't parameters to get_sqla_query
+ sqla_query_keys = {
+ "apply_fetch_values_predicate",
+ "columns",
+ "extras",
+ "filter",
+ "from_dttm",
+ "granularity",
+ "groupby",
+ "inner_from_dttm",
+ "inner_to_dttm",
+ "is_rowcount",
+ "is_timeseries",
+ "metrics",
+ "orderby",
+ "order_desc",
+ "to_dttm",
+ "series_columns",
+ "series_limit",
+ "series_limit_metric",
+ "group_others_when_limit_reached",
+ "row_limit",
+ "row_offset",
+ "timeseries_limit",
+ "timeseries_limit_metric",
+ "time_shift",
+ }
Review Comment:
The `sqla_query_keys` set is duplicated in both `get_query_str_extended`
(here) and in `superset/connectors/sqla/models.py` in the
`SqlaTable.get_extra_cache_keys` method (lines 1924-1949). Consider extracting
this set to a module-level constant or a shared helper method to avoid
maintaining two copies of the same parameter list.
##########
superset/connectors/sqla/models.py:
##########
@@ -1912,7 +1920,37 @@ class and any keys added via `ExtraCache`.
"""
extra_cache_keys = super().get_extra_cache_keys(query_obj)
if self.has_extra_cache_key_calls(query_obj):
- sqla_query = self.get_sqla_query(**query_obj)
+ # Filter out keys that aren't parameters to get_sqla_query
+ sqla_query_keys = {
+ "apply_fetch_values_predicate",
+ "columns",
+ "extras",
+ "filter",
+ "from_dttm",
+ "granularity",
+ "groupby",
+ "inner_from_dttm",
+ "inner_to_dttm",
+ "is_rowcount",
+ "is_timeseries",
+ "metrics",
+ "orderby",
+ "order_desc",
+ "to_dttm",
+ "series_columns",
+ "series_limit",
+ "series_limit_metric",
+ "group_others_when_limit_reached",
+ "row_limit",
+ "row_offset",
+ "timeseries_limit",
+ "timeseries_limit_metric",
+ "time_shift",
+ }
Review Comment:
The `sqla_query_keys` set is duplicated in both `get_extra_cache_keys`
(here) and in `superset/models/helpers.py` in the
`BaseDatasource.get_query_str_extended` method (lines 978-1003). Consider
extracting this set to a module-level constant or a shared helper method to
avoid maintaining two copies of the same parameter list.
##########
superset/connectors/sqla/models.py:
##########
@@ -470,14 +475,15 @@ def data_for_slices( # pylint: disable=too-many-locals
# noqa: C901
or metric["verbose_name"] in metric_names
]
- filtered_columns: list[Column] = []
+ filtered_columns: list[dict[str, Any]] = []
column_types: set[utils.GenericDataType] = set()
- for column_ in data["columns"]:
- generic_type = column_.get("type_generic")
+ for column_ in cast(list[dict[str, Any]], data["columns"]): # type:
ignore[assignment]
+ column_dict = cast(dict[str, Any], column_)
Review Comment:
The cast on this line is redundant since `column_` is already typed as
`dict[str, Any]` from the cast on line 480. The variable can be used directly
without the additional cast.
```suggestion
column_dict = column_
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
