gabotorresruiz commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r3825285636
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -185,6 +187,11 @@ def _sql_from_saved_query_context(
query_context = ChartDataQueryContextSchema().load(qc_json)
query_context.result_type = ChartDataResultType.QUERY
+ set_query_context_form_data(
+ query_context,
+ chart.datasource_id,
+ chart.datasource_type,
+ )
Review Comment:
Not a blocker, a parity question: here the Jinja fallback gets
`chart.datasource_id`, but the command executes the datasource encoded in the
saved `query_context`. If a chart is repointed to another dataset while its
saved `query_context` is stale, `get_dataset_id_from_context()` (and so an
implicit `metric()` lookup) resolves a different dataset than the SQL we
return. Since `query_context` is already loaded here,
`query_context.datasource.id` and `str(query_context.datasource.type)` would
make the template context and the execution target unable to diverge, matching
the fallback you already added in `_sql_from_form_data`. Same question for the
saved-context branch in `get_chart_data` (line 681), where
`merge_extra_form_data_filters_into_query` a few lines up already uses
`query_context_json["datasource"]["id"]`. Or is the chart-level datasource
intentional there?
--
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]
