[GitHub] [superset] ktmud commented on a change in pull request #12482: refactor(connectors): refactor sqla query generation

[GitBox <gi...@apache.org>]

ktmud commented on a change in pull request #12482:
URL: https://github.com/apache/superset/pull/12482#discussion_r559044756



##########
File path: superset/connectors/sqla/models.py
##########
@@ -871,102 +908,76 @@ def _get_sqla_row_level_filters(
                 _("Error in jinja expression in RLS filters: %(msg)s", msg=ex.message,)
             )
 
-    def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-many-branches,too-many-statements
-        self,
-        metrics: List[Metric],
-        granularity: str,
-        from_dttm: Optional[datetime],
-        to_dttm: Optional[datetime],
-        columns: Optional[List[str]] = None,
-        groupby: Optional[List[str]] = None,
-        filter: Optional[  # pylint: disable=redefined-builtin
-            List[Dict[str, Any]]
-        ] = None,
-        is_timeseries: bool = True,
-        timeseries_limit: int = 15,
-        timeseries_limit_metric: Optional[Metric] = None,
-        row_limit: Optional[int] = None,
-        row_offset: Optional[int] = None,
-        inner_from_dttm: Optional[datetime] = None,
-        inner_to_dttm: Optional[datetime] = None,
-        orderby: Optional[List[Tuple[ColumnElement, bool]]] = None,
-        extras: Optional[Dict[str, Any]] = None,
-        order_desc: bool = True,
-    ) -> SqlaQuery:
-        """Querying any sqla table from this common interface"""
-        template_kwargs = {
-            "from_dttm": from_dttm.isoformat() if from_dttm else None,
-            "groupby": groupby,
-            "metrics": metrics,
-            "row_limit": row_limit,
-            "row_offset": row_offset,
-            "to_dttm": to_dttm.isoformat() if to_dttm else None,
-            "filter": filter,
-            "columns": [col.column_name for col in self.columns],
-        }
-        is_sip_38 = is_feature_enabled("SIP_38_VIZ_REARCHITECTURE")
+    def _update_template_kwargs(self, template_kwargs: Dict[str, Any]) -> None:
         template_kwargs.update(self.template_params_dict)
-        extra_cache_keys: List[Any] = []
-        template_kwargs["extra_cache_keys"] = extra_cache_keys
-        template_processor = self.get_template_processor(**template_kwargs)
-        db_engine_spec = self.database.db_engine_spec
-        prequeries: List[str] = []
 
-        orderby = orderby or []
+    def _get_columns_by_name(self) -> Dict[str, TableColumn]:
+        return {col.column_name: col for col in self.columns}
 
-        # For backward compatibility
-        if granularity not in self.dttm_cols:
-            granularity = self.main_dttm_col
+    def _get_metrics_by_name(self) -> Dict[str, SqlMetric]:
+        return {m.metric_name: m for m in self.metrics}
 
-        # Database spec supports join-free timeslot grouping
-        time_groupby_inline = db_engine_spec.time_groupby_inline
-
-        columns_by_name: Dict[str, TableColumn] = {
-            col.column_name: col for col in self.columns
-        }
-        metrics_by_name: Dict[str, SqlMetric] = {m.metric_name: m for m in self.metrics}
-
-        if not granularity and is_timeseries:
-            raise QueryObjectValidationError(
-                _(
-                    "Datetime column not provided as part table configuration "
-                    "and is required by this type of chart"
-                )
-            )
-        if (
-            not metrics
-            and not columns
-            and (is_sip_38 or (not is_sip_38 and not groupby))
-        ):
-            raise QueryObjectValidationError(_("Empty query?"))
-        metrics_exprs: List[ColumnElement] = []
+    def _get_metric_expressions(
+        self,
+        metrics: List[Metric],
+        columns_by_name: Dict[str, TableColumn],
+        metrics_by_name: Dict[str, SqlMetric],
+    ) -> List[Label]:

Review comment:
       I wonder how many of these private methods can be move out of the class. We should also try not to pass too many arguments between functions.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org