bito-code-review[bot] commented on code in PR #44948:
URL: https://github.com/apache/superset/pull/44948#discussion_r4173694826
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -1049,6 +1077,9 @@ async def execute_chart_data( # noqa: C901
columns=columns,
data=data[: request.limit] if request.limit else data,
query_results=_build_query_results(result["queries"],
request.limit),
+ headline=_big_number_headline(
+ chart_viz_type, form_data, result["queries"], query_context
Review Comment:
<!-- Bito Reply -->
The suggestion to guard `form_data` is not necessary because `dict.get()` is
a safe method that does not raise an `AttributeError` when called on an empty
dictionary. As noted, the existing logic already ensures `form_data` is a
dictionary, and regression tests confirm that empty `form_data` is handled
correctly without losing chart data.
##########
superset/mcp_service/chart/big_number_headline.py:
##########
@@ -0,0 +1,344 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Headline value of Big Number charts.
+
+A Big Number chart renders one number. For the trendline variant
(``big_number``)
+that number is derived client-side from the time series; for
``big_number_total``
+it is the single metric value. Neither is any of the first sample rows, so this
+module reproduces the frontend computation:
+
+- ``aggregationChoices`` in ``superset-ui-chart-controls``
(``customControls.tsx``)
+- ``BigNumberWithTrendline/transformProps.ts`` and
``BigNumberTotal/transformProps.ts``
+
+A headline is only returned when it is exact. Otherwise ``value`` is null with
a
+``reason``: a wrong number is worse than none.
+"""
+
+from __future__ import annotations
+
+import math
+import statistics
+from collections.abc import Callable, Mapping, Sequence
+from datetime import date, datetime, timezone
+from decimal import Decimal
+from typing import Any
+
+from superset.mcp_service.chart.schemas import BigNumberHeadline
+from superset.utils.core import DTTM_ALIAS, get_metric_name
+
+BIG_NUMBER_TRENDLINE_VIZ_TYPE = "big_number"
+BIG_NUMBER_TOTAL_VIZ_TYPE = "big_number_total"
+
+DEFAULT_AGGREGATION = "LAST_VALUE"
+RAW_AGGREGATION = "raw"
+
+# Metric-value transforms for the trend series. Keys and order mirror the
+# frontend's `aggregationChoices`. `LAST_VALUE` and `raw` receive values
ordered
+# newest first and take the first, so they need no entry beyond that.
+_AGGREGATIONS: dict[str, Callable[[list[float]], float | None]] = {
+ "raw": lambda values: values[0] if values else None,
+ "LAST_VALUE": lambda values: values[0] if values else None,
+ "sum": lambda values: sum(values) if values else None,
+ "mean": lambda values: sum(values) / len(values) if values else None,
+ "min": lambda values: min(values) if values else None,
+ "max": lambda values: max(values) if values else None,
+ "median": lambda values: statistics.median(values) if values else None,
+}
+
+# Rolling types that make the query add a `rolling` (sum, mean, std) or `cum`
+# (cumsum) post-processing step, as in the frontend's `rollingWindowOperator`.
+_ROLLING_OPERATIONS = {
+ "cumsum": "cum",
+ "sum": "rolling",
+ "mean": "rolling",
+ "std": "rolling",
+}
+
+
+def is_big_number_viz_type(viz_type: str | None) -> bool:
+ return viz_type in (BIG_NUMBER_TRENDLINE_VIZ_TYPE,
BIG_NUMBER_TOTAL_VIZ_TYPE)
+
+
+def executed_query_facts(query_context: Any) -> tuple[int | None, list[str]]:
+ """Row limit and post-processing operations of the first executed query.
+
+ Read from the query that actually ran, so it reflects any row-limit
override
+ and shows whether the chart's advanced analytics were part of the query.
+ """
+ queries = getattr(query_context, "queries", None) or []
+ if not queries:
+ return None, []
+ first = queries[0]
+ row_limit = getattr(first, "row_limit", None)
+ operations = [
+ str(step.get("operation"))
+ for step in getattr(first, "post_processing", None) or []
+ if isinstance(step, Mapping) and step.get("operation")
+ ]
+ return (row_limit if isinstance(row_limit, int) else None), operations
+
+
+def _unavailable(aggregation: str | None, reason: str) -> BigNumberHeadline:
+ return BigNumberHeadline(value=None, aggregation=aggregation,
reason=reason)
+
+
+def _parse_date_ms(value: str) -> int | None:
+ """Epoch milliseconds for an ISO-8601 string, else None (frontend:
+ strict `dayjs.utc` parse in `parseMetricValue`)."""
+ try:
+ parsed = datetime.fromisoformat(value)
+ except ValueError:
+ return None
+ if parsed.tzinfo is None:
+ parsed = parsed.replace(tzinfo=timezone.utc)
+ return int(parsed.timestamp() * 1000)
+
+
+def _parse_metric_value(value: Any) -> int | float | None:
+ """Mirror of the frontend's `parseMetricValue`, plus JSON-safety: numbers
+ pass through, date strings become epoch ms, anything else (including NaN
and
+ infinities, which serialize as null) is null."""
+ if isinstance(value, bool) or value is None:
+ return None
+ if isinstance(value, Decimal):
+ value = float(value)
+ if isinstance(value, (int, float)):
+ return value if math.isfinite(value) else None
Review Comment:
<!-- Bito Reply -->
The suggestion to guard integers before the `math.isfinite` check is
correct. `math.isfinite` raises an `OverflowError` when encountering integers
that exceed the floating-point range, which would cause the
`_parse_metric_value` function to crash. Applying this guard ensures the
function returns `None` as intended for such values, maintaining the robustness
of the chart data processing.
**superset/mcp_service/chart/big_number_headline.py**
```
if isinstance(value, Decimal):
value = float(value)
if isinstance(value, int):
return value
if isinstance(value, float):
return value if math.isfinite(value) else None
```
##########
superset/mcp_service/chart/big_number_headline.py:
##########
@@ -0,0 +1,344 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Headline value of Big Number charts.
+
+A Big Number chart renders one number. For the trendline variant
(``big_number``)
+that number is derived client-side from the time series; for
``big_number_total``
+it is the single metric value. Neither is any of the first sample rows, so this
+module reproduces the frontend computation:
+
+- ``aggregationChoices`` in ``superset-ui-chart-controls``
(``customControls.tsx``)
+- ``BigNumberWithTrendline/transformProps.ts`` and
``BigNumberTotal/transformProps.ts``
+
+A headline is only returned when it is exact. Otherwise ``value`` is null with
a
+``reason``: a wrong number is worse than none.
+"""
+
+from __future__ import annotations
+
+import math
+import statistics
+from collections.abc import Callable, Mapping, Sequence
+from datetime import date, datetime, timezone
+from decimal import Decimal
+from typing import Any
+
+from superset.mcp_service.chart.schemas import BigNumberHeadline
+from superset.utils.core import DTTM_ALIAS, get_metric_name
+
+BIG_NUMBER_TRENDLINE_VIZ_TYPE = "big_number"
+BIG_NUMBER_TOTAL_VIZ_TYPE = "big_number_total"
+
+DEFAULT_AGGREGATION = "LAST_VALUE"
+RAW_AGGREGATION = "raw"
+
+# Metric-value transforms for the trend series. Keys and order mirror the
+# frontend's `aggregationChoices`. `LAST_VALUE` and `raw` receive values
ordered
+# newest first and take the first, so they need no entry beyond that.
+_AGGREGATIONS: dict[str, Callable[[list[float]], float | None]] = {
+ "raw": lambda values: values[0] if values else None,
+ "LAST_VALUE": lambda values: values[0] if values else None,
+ "sum": lambda values: sum(values) if values else None,
+ "mean": lambda values: sum(values) / len(values) if values else None,
+ "min": lambda values: min(values) if values else None,
+ "max": lambda values: max(values) if values else None,
+ "median": lambda values: statistics.median(values) if values else None,
+}
+
+# Rolling types that make the query add a `rolling` (sum, mean, std) or `cum`
+# (cumsum) post-processing step, as in the frontend's `rollingWindowOperator`.
+_ROLLING_OPERATIONS = {
+ "cumsum": "cum",
+ "sum": "rolling",
+ "mean": "rolling",
+ "std": "rolling",
+}
+
+
+def is_big_number_viz_type(viz_type: str | None) -> bool:
+ return viz_type in (BIG_NUMBER_TRENDLINE_VIZ_TYPE,
BIG_NUMBER_TOTAL_VIZ_TYPE)
Review Comment:
<!-- Bito Reply -->
The docstring for `is_big_number_viz_type` has been added in the latest
commit (0cb0e41b76fadb2aaf3c7b22c3b3f837348eabf9), addressing the review
suggestion. The function now includes the following docstring:
```python
def is_big_number_viz_type(viz_type: str | None) -> bool:
"""Whether the visualization displays a Big Number headline."""
return viz_type in (BIG_NUMBER_TRENDLINE_VIZ_TYPE,
BIG_NUMBER_TOTAL_VIZ_TYPE)
```
This change ensures consistency with the module's documentation standards.
**superset/mcp_service/chart/big_number_headline.py**
```
def is_big_number_viz_type(viz_type: str | None) -> bool:
"""Whether the visualization displays a Big Number headline."""
return viz_type in (BIG_NUMBER_TRENDLINE_VIZ_TYPE,
BIG_NUMBER_TOTAL_VIZ_TYPE)
```
--
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]
