[GitHub] [superset] ktmud commented on a change in pull request #16660: [WIP] feat: add support for generic series limit

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

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



##########
File path: superset/common/query_object.py
##########
@@ -74,63 +77,68 @@ class QueryObject:  # pylint: disable=too-many-instance-attributes
     annotation_layers: List[Dict[str, Any]]
     applied_time_extras: Dict[str, str]
     apply_fetch_values_predicate: bool
-    granularity: Optional[str]
+    columns: List[str]
+    datasource: Optional[BaseDatasource]
+    extras: Dict[str, Any]
+    filter: List[QueryObjectFilterClause]
     from_dttm: Optional[datetime]
-    to_dttm: Optional[datetime]
+    granularity: Optional[str]
     inner_from_dttm: Optional[datetime]
     inner_to_dttm: Optional[datetime]
+    is_rowcount: bool
     is_timeseries: bool
-    time_shift: Optional[timedelta]
-    groupby: List[str]
-    metrics: Optional[List[Metric]]
-    row_limit: int
-    row_offset: int
-    filter: List[QueryObjectFilterClause]
-    timeseries_limit: int
-    timeseries_limit_metric: Optional[Metric]
     order_desc: bool
-    extras: Dict[str, Any]
-    columns: List[str]
     orderby: List[OrderBy]
-    post_processing: List[Dict[str, Any]]
-    datasource: Optional[BaseDatasource]
+    metrics: Optional[List[Metric]]
     result_type: Optional[ChartDataResultType]
-    is_rowcount: bool
+    row_limit: int
+    row_offset: int
+    series_columns: List[str]
+    series_limit: int
+    series_limit_metric: Optional[Metric]
     time_offsets: List[str]
+    time_shift: Optional[timedelta]
+    to_dttm: Optional[datetime]
+    post_processing: List[Dict[str, Any]]
 
     def __init__(  # pylint: disable=too-many-arguments,too-many-locals
         self,
-        datasource: Optional[DatasourceDict] = None,
-        result_type: Optional[ChartDataResultType] = None,
         annotation_layers: Optional[List[Dict[str, Any]]] = None,
         applied_time_extras: Optional[Dict[str, str]] = None,
         apply_fetch_values_predicate: bool = False,
-        granularity: Optional[str] = None,
-        metrics: Optional[List[Metric]] = None,
-        groupby: Optional[List[str]] = None,
+        columns: Optional[List[str]] = None,
+        datasource: Optional[DatasourceDict] = None,
+        extras: Optional[Dict[str, Any]] = None,
         filters: Optional[List[QueryObjectFilterClause]] = None,
-        time_range: Optional[str] = None,
-        time_shift: Optional[str] = None,
+        granularity: Optional[str] = None,
+        is_rowcount: bool = False,
         is_timeseries: Optional[bool] = None,
-        timeseries_limit: int = 0,
-        row_limit: Optional[int] = None,
-        row_offset: Optional[int] = None,
-        timeseries_limit_metric: Optional[Metric] = None,
+        metrics: Optional[List[Metric]] = None,
         order_desc: bool = True,
-        extras: Optional[Dict[str, Any]] = None,
-        columns: Optional[List[str]] = None,
         orderby: Optional[List[OrderBy]] = None,
         post_processing: Optional[List[Optional[Dict[str, Any]]]] = None,
-        is_rowcount: bool = False,
+        result_type: Optional[ChartDataResultType] = None,
+        row_limit: Optional[int] = None,
+        row_offset: Optional[int] = None,
+        series_columns: Optional[List[str]] = None,
+        series_limit: int = 0,
+        series_limit_metric: Optional[Metric] = None,
+        time_range: Optional[str] = None,
+        time_shift: Optional[str] = None,
         **kwargs: Any,
     ):
         columns = columns or []
-        groupby = groupby or []
         extras = extras or {}
         annotation_layers = annotation_layers or []
         self.time_offsets = kwargs.get("time_offsets", [])
         self.inner_from_dttm = kwargs.get("inner_from_dttm")
         self.inner_to_dttm = kwargs.get("inner_to_dttm")
+        if series_columns:
+            self.series_columns = series_columns
+        elif is_timeseries and metrics:
+            self.series_columns = columns
+        else:
+            self.series_columns = []

Review comment:
       ```python
   self.series_columns = series_columns or (columns if is_timeseries and metrics else [])
   ```
   
   Would this be more Pythonic?
   
   I'm also wondering whether we should have another layer of parameter consolidation before `QueryObject` so to handle all the special overrides & fallbacks for legacy/deprecated parameters (e.g. `timeseries_limit` vs `series_limit`, `columns` vs `groupby`, `sortby` vs `orderby`). By isolating parameter consolidation in the Flask view handler layer, we reduce the number of parameters for `QueryObject` itself and simply all downstream functions, which may help cleaning up deprecated code faster---all without affecting backward compatibility.




-- 
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: notifications-unsubscribe@superset.apache.org

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