bito-code-review[bot] commented on code in PR #44948:
URL: https://github.com/apache/superset/pull/44948#discussion_r4173570786
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>AttributeError on empty form_data</b></div>
<div id="fix">
`_big_number_headline` passes `form_data`, which on the saved-chart path can
be `{}` (lines 630-640: unparsable `chart_params` yield `{}`) while
`chart_viz_type` (line 523) selects the big_number branch.
`compute_big_number_headline` then evaluates `str(form_data.get("aggregation")
or "")` (big_number_headline.py:307), raising `AttributeError: 'dict' object
has no attribute 'get'` only when `form_data` is not a dict. The outer handler
turns this into a generic InternalError, losing the chart data. Guard the
empty/non-dict form_data before computing.
</div>
</div>
<small><i>Code Review Run #4a861d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing docstring on new function</b></div>
<div id="fix">
`is_big_number_viz_type` is the only function in this new module without a
docstring (AST check: 11 of 14 functions have one). BITO.md adaptive rule 12147
requires an inline docstring on every newly introduced Python function. A
one-liner documenting the predicate's intent keeps the module consistent.
</div>
</div>
<small><i>Code Review Run #4a861d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>OverflowError on huge ints</b></div>
<div id="fix">
`math.isfinite` raises `OverflowError` for ints beyond float range (verified
on the repo's Python 3.11.2), so a huge metric value from query rows (e.g. a
NUMERIC column) crashes `_parse_metric_value` and the whole `get_chart_data`
tool call instead of yielding the null the docstring promises. Guard ints
before the float check.
</div>
</div>
<small><i>Code Review Run #4a861d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
