codeant-ai-for-open-source[bot] commented on code in PR #41895:
URL: https://github.com/apache/superset/pull/41895#discussion_r3546640467
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -911,6 +929,17 @@ def map_big_number_config(config: BigNumberChartConfig) ->
Dict[str, Any]:
_add_adhoc_filters(form_data, config.filters)
+ # Bind a TEMPORAL_RANGE adhoc filter so dashboard time-range filters have
+ # a column to apply to — mirrors the Explore UI's `BigNumberTotal` control
+ # panel, which exposes an `adhoc_filters` control even though there's no
+ # dedicated time-column control for the total variant. Falls back to the
+ # dataset's main_dttm_col when the caller didn't specify temporal_column.
+ # Guarded by is_column_truly_temporal (same check map_xy_config applies to
+ # its x-axis) so a non-temporal column never gets a TEMPORAL_RANGE filter.
+ temporal_column = config.temporal_column or
_find_dataset_main_dttm_col(dataset_id)
+ if temporal_column and is_column_truly_temporal(temporal_column,
dataset_id):
+ _ensure_temporal_adhoc_filter(form_data, temporal_column)
Review Comment:
**Suggestion:** This introduces two dataset lookups for the same request
path when `temporal_column` is omitted (`_find_dataset_main_dttm_col` fetches
the dataset, then `is_column_truly_temporal` fetches it again). On chart
generation hot paths this doubles database round-trips and adds avoidable
latency/load; fetch the dataset once and reuse it for both main datetime
resolution and temporal-type validation. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP generate_explore_link Big Number path re-queries dataset.
- ⚠️ Metadata database load increases for repeated Big Number charts.
- ⚠️ Added latency for dashboard time-filtered Big Number totals.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP `generate_explore_link` tool with a Big Number chart
configuration that
has `chart_type='big_number'`, `show_trendline=False`, and no
`temporal_column`, and a
valid `dataset_id` that points to a dataset with `main_dttm_col` (entry path
at
`superset/mcp_service/explore/tool/generate_explore_link.py:63-71` and
request schema in
the same module).
2. In `generate_explore_link`, the dataset is first loaded and validated via
`DatasetDAO.find_by_id(...)` in the `dataset_check` block at
`superset/mcp_service/explore/tool/generate_explore_link.py:8-20`, which
issues one
metadata DB query using the DAO implementation in
`superset/daos/base.py:29-61`.
3. The code then maps the chart config to form_data by calling
`map_config_to_form_data(normalized_config, dataset_id=request.dataset_id)`
at
`superset/mcp_service/explore/tool/generate_explore_link.py:35-38`, which
resolves the
`BigNumberChartPlugin` and calls its `to_form_data` method
(`superset/mcp_service/chart/plugins/big_number.py:151-155`), delegating to
`map_big_number_config` in
`superset/mcp_service/chart/chart_utils.py:45-104`.
4. Inside `map_big_number_config`, because `config.temporal_column` is falsy
for this Big
Number total case, the line at `chart_utils.py:939` calls
`_find_dataset_main_dttm_col(dataset_id)`, which itself calls
`_find_dataset_by_id_or_uuid(dataset_id)` at `chart_utils.py:873-879`,
resulting in a
`DatasetDAO.find_by_id(...)` query (second dataset lookup); immediately
after, the `if
temporal_column and is_column_truly_temporal(temporal_column, dataset_id):`
guard at
`chart_utils.py:940-941` invokes `is_column_truly_temporal`, which again
calls
`_find_dataset_by_id_or_uuid(dataset_id)` at `chart_utils.py:287-315`,
causing another
`DatasetDAO.find_by_id(...)` query (third lookup). These two DAO calls inside
`map_big_number_config` are redundant for the same `dataset_id` and request,
doubling
metadata DB round-trips on this code path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ec0ed18584d243a49a1ae2a0607a1997&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ec0ed18584d243a49a1ae2a0607a1997&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/chart_utils.py
**Line:** 939:941
**Comment:**
*Performance: This introduces two dataset lookups for the same request
path when `temporal_column` is omitted (`_find_dataset_main_dttm_col` fetches
the dataset, then `is_column_truly_temporal` fetches it again). On chart
generation hot paths this doubles database round-trips and adds avoidable
latency/load; fetch the dataset once and reuse it for both main datetime
resolution and temporal-type validation.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41895&comment_hash=9ab096aa3852270cc523d8dd150d137757a3024351e50cc0b61b60e4fe5535ef&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41895&comment_hash=9ab096aa3852270cc523d8dd150d137757a3024351e50cc0b61b60e4fe5535ef&reaction=dislike'>👎</a>
--
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]
