feat: add support for generic series limit #16660
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,9 @@ class DeprecatedField(NamedTuple): | |
|
|
||
| DEPRECATED_FIELDS = ( | ||
| DeprecatedField(old_name="granularity_sqla", new_name="granularity"), | ||
| DeprecatedField(old_name="groupby", new_name="columns"), | ||
| DeprecatedField(old_name="timeseries_limit", new_name="series_limit"), | ||
| DeprecatedField(old_name="timeseries_limit_metric", new_name="series_limit_metric"), | ||
| ) | ||
|
|
||
| DEPRECATED_EXTRAS_FIELDS = ( | ||
|
|
@@ -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 | ||
| columns: List[str] | ||
| datasource: Optional[BaseDatasource] | ||
| extras: Dict[str, Any] | ||
| filter: List[QueryObjectFilterClause] | ||
| from_dttm: Optional[datetime] | ||
| granularity: Optional[str] | ||
| inner_from_dttm: Optional[datetime] | ||
| inner_to_dttm: Optional[datetime] | ||
| is_rowcount: bool | ||
| is_timeseries: bool | ||
| order_desc: bool | ||
| orderby: List[OrderBy] | ||
| metrics: Optional[List[Metric]] | ||
| result_type: Optional[ChartDataResultType] | ||
| 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, | ||
| annotation_layers: Optional[List[Dict[str, Any]]] = None, | ||
| applied_time_extras: Optional[Dict[str, str]] = None, | ||
| apply_fetch_values_predicate: bool = False, | ||
| columns: Optional[List[str]] = None, | ||
| datasource: Optional[DatasourceDict] = None, | ||
| extras: Optional[Dict[str, Any]] = None, | ||
| filters: Optional[List[QueryObjectFilterClause]] = None, | ||
| granularity: Optional[str] = None, | ||
| is_rowcount: bool = False, | ||
| is_timeseries: Optional[bool] = None, | ||
| metrics: Optional[List[Metric]] = None, | ||
| order_desc: bool = True, | ||
| orderby: Optional[List[OrderBy]] = None, | ||
| post_processing: Optional[List[Optional[Dict[str, Any]]]] = None, | ||
| 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 [] | ||
| 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 = [] | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment.
IMO the current implementation is more pythonic/readable, as it doesn't require unnesting the inline logic when reading it. But I'm ok with the proposed snippet, too. I'm curious to hear what @dpgaspar thinks - it would be nice to add something on this to the code style guide.
I wanted to restrict this PR to the bare minimum amount of changes to contain the blast radius (whenever we're touching this type of core logic there's serious risk of regressions). I absolutely agree we should consolidate more (and should finish the consolidation work before cutting 2.0), but I'd propose doing it across multiple PRs to avoid making one big bang PR. There was a problem hiding this 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. |
||
|
|
||
| self.is_rowcount = is_rowcount | ||
| self.datasource = None | ||
|
|
@@ -161,9 +169,7 @@ def __init__( # pylint: disable=too-many-arguments,too-many-locals | |
| # is_timeseries is True if time column is in either columns or groupby | ||
| # (both are dimensions) | ||
| self.is_timeseries = ( | ||
| is_timeseries if is_timeseries is not None else DTTM_ALIAS in columns | ||
| ) | ||
| self.time_range = time_range | ||
| self.time_shift = parse_human_timedelta(time_shift) | ||
|
|
@@ -183,8 +189,8 @@ def __init__( # pylint: disable=too-many-arguments,too-many-locals | |
| self.row_limit = config["ROW_LIMIT"] if row_limit is None else row_limit | ||
| self.row_offset = row_offset or 0 | ||
| self.filter = filters or [] | ||
| self.series_limit = series_limit | ||
| self.series_limit_metric = series_limit_metric | ||
| self.order_desc = order_desc | ||
| self.extras = extras | ||
|
|
||
|
|
@@ -194,9 +200,12 @@ def __init__( # pylint: disable=too-many-arguments,too-many-locals | |
| ) | ||
|
|
||
| self.columns = columns | ||
| self.orderby = orderby or [] | ||
|
|
||
| self._rename_deprecated_fields(kwargs) | ||
| self._move_deprecated_extra_fields(kwargs) | ||
|
|
||
| def _rename_deprecated_fields(self, kwargs: Dict[str, Any]) -> None: | ||
| # rename deprecated fields | ||
| for field in DEPRECATED_FIELDS: | ||
| if field.old_name in kwargs: | ||
|
|
@@ -216,6 +225,7 @@ def __init__( # pylint: disable=too-many-arguments,too-many-locals | |
| ) | ||
| setattr(self, field.new_name, value) | ||
|
|
||
| def _move_deprecated_extra_fields(self, kwargs: Dict[str, Any]) -> None: | ||
| # move deprecated extras fields to extras | ||
| for field in DEPRECATED_EXTRAS_FIELDS: | ||
| if field.old_name in kwargs: | ||
|
|
@@ -254,6 +264,14 @@ def validate( | |
| """Validate query object""" | ||
| error: Optional[QueryObjectValidationError] = None | ||
| all_labels = self.metric_names + self.column_names | ||
| missing_series = [col for col in self.series_columns if col not in self.columns] | ||
| if missing_series: | ||
| _( | ||
|
|
||
| "The following entries in `series_columns` are missing " | ||
| "in `columns`: %(columns)s. ", | ||
| columns=", ".join(f'"{x}"' for x in missing_series), | ||
| ) | ||
|
|
||
| if len(set(all_labels)) < len(all_labels): | ||
| dup_labels = find_duplicates(all_labels) | ||
| error = QueryObjectValidationError( | ||
|
|
@@ -270,24 +288,24 @@ def validate( | |
| def to_dict(self) -> Dict[str, Any]: | ||
| query_object_dict = { | ||
| "apply_fetch_values_predicate": self.apply_fetch_values_predicate, | ||
| "columns": self.columns, | ||
| "extras": self.extras, | ||
| "filter": self.filter, | ||
| "from_dttm": self.from_dttm, | ||
| "granularity": self.granularity, | ||
| "inner_from_dttm": self.inner_from_dttm, | ||
| "inner_to_dttm": self.inner_to_dttm, | ||
| "is_rowcount": self.is_rowcount, | ||
| "is_timeseries": self.is_timeseries, | ||
| "metrics": self.metrics, | ||
| "order_desc": self.order_desc, | ||
| "orderby": self.orderby, | ||
| "row_limit": self.row_limit, | ||
| "row_offset": self.row_offset, | ||
| "series_columns": self.series_columns, | ||
| "series_limit": self.series_limit, | ||
| "series_limit_metric": self.series_limit_metric, | ||
| "to_dttm": self.to_dttm, | ||
| } | ||
| return query_object_dict | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In hindsight I should not have reordered these in this PR to keep it as easily reviewable as possible (my sincere apologies to reviewers!), but having these non-ordered annoyed me so much I couldn't help myself. The point here is I've added
series_columns,series_limitandseries_limit_metricand removedtimeseries_limitandtimeseries_limit_metricas they're picked up bykwargsand handled in the deprecated mapping methods.