richardfogaca commented on PR #40099:
URL: https://github.com/apache/superset/pull/40099#issuecomment-4446362880
**Code review — on behalf of Amin Ghadersohi (routed by EngCodeReviewBot)**
Overall the approach is correct and the DRY refactor is clean. The shared
normalization pipeline mirrors what Explore/viz.py does, and the test coverage
is solid. A few things to verify or address:
---
**1. `apply_form_data_filters_to_query` now propagates `time_range` — is
that intentional?**
The previous code only forwarded `filters` and `adhoc_filters` into the
query dict. The new `apply_form_data_filters_to_query` also copies
`time_range`. That's a silent behavior change for every call site. If
`time_range` was already handled elsewhere in `QueryContextFactory` (e.g. via
`form_data=`), this could double-apply it. Worth adding a note or test
confirming the expected behavior.
**2. Import inside function body in `get_chart_sql.py`**
```python
# _build_single_query_dict, near the end
from superset.mcp_service.chart.chart_helpers import (
apply_form_data_filters_to_query,
)
apply_form_data_filters_to_query(qd, form_data)
```
This import is inside the function, inconsistent with the rest of the file.
Move it to the top-level imports with the other `chart_helpers` imports.
**3. Potential redundancy in `_build_single_query_dict`**
`_build_single_query_dict` already writes `qd["filters"]` from its own
`filters` variable, and then `apply_form_data_filters_to_query` overwrites
`qd["filters"]` with `form_data.get("filters")`. If both come from the same
normalized `form_data`, the second write is redundant but harmless. If they can
diverge (e.g. `filters` variable comes from somewhere else), the second write
silently wins. Worth a quick check.
**4. `prepare_form_data_for_query` mutates input — document it**
The function modifies `form_data` in-place. Current callers are all fine
(fresh dicts per path), but the docstring doesn't mention mutation. Future
callers could get surprised. Either add `# mutates form_data in-place` to the
docstring or rename to `normalize_form_data_for_query` to make it obvious.
**5. `get_chart_preview` paths don't pass `extra_form_data`**
`prepare_form_data_for_query` is called without `extra_form_data` in all
preview strategies. This is pre-existing (not a regression from this PR), but
worth a follow-up issue if preview tools should also support dashboard native
filters.
---
The core fix in `chart_helpers.py` and the refactor of the
cached/fallback/saved paths in `get_chart_data.py` are all correct. Items 1–3
above are the ones most worth resolving before merge.
--
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]
