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

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

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



##########
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:
       I have no strong opinions here, but, I do like to use simple ternaries. But current implementation seems more readable to me because of the nested ternary.
   




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