sadpandajoe commented on code in PR #43771:
URL: https://github.com/apache/superset/pull/43771#discussion_r4192908316
##########
superset/mcp_service/chart/plugins/table.py:
##########
@@ -44,6 +46,12 @@ class TableChartPlugin(BaseChartPlugin):
}
supports_column_append = True
+ def prepare_query_form_data(self, form_data: dict[str, Any]) -> None:
+ """Resolve inherited offsets before the filter merge removes their
source."""
+ inherited = (form_data.get("extra_form_data") or
{}).get("time_compare")
+ if inherited and inherited not in
as_list(form_data.get("time_compare") or []):
Review Comment:
This replaces the whole selection with the inherited offset whenever it
isn't literally in the raw `time_compare` list, but `_table_time_offsets` only
compares it after resolving `custom`. A saved aggregate Table with
`time_compare=["1 year ago", "custom"]`, `start_date_offset="2 weeks ago"` and
`extra_form_data={"time_compare": "2 weeks ago"}` becomes `["2 weeks ago"]`
here, so the year-over-year comparison column is dropped even though the
frontend keeps both. Could the hook resolve custom offsets before the
membership check, or leave the replacement to `_table_time_offsets`?
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -53,6 +58,17 @@ class XYChartPlugin(BaseChartPlugin):
"echarts_area": "Area Chart",
"echarts_timeseries_scatter": "Scatter Plot",
}
+ query_role_keys = BaseChartPlugin.query_role_keys | {"x_axis"}
+
+ def vega_lite_preview(
Review Comment:
This hook now claims saved XY previews, but the plugin still has
`resizes_saved_preview=False`, so `_create_plugin_preview` skips the
width/height/description framing that the saved-chart fallback applied before.
A saved `echarts_timeseries_line` preview requested with `width=800,
height=200` used to honor those dimensions and now comes back as
`width="container", height=400`. Should `resizes_saved_preview` be `True` for
XY, as Gantt does?
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -1452,33 +1671,35 @@ def _generate_vega_lite_preview_from_data( # noqa: C901
mark = viz_to_mark.get(viz_type, "bar")
# Basic Vega-Lite spec
- spec = {
+ preview_data = _bounded_vega_data(data)
+ spec: dict[str, Any] = {
"$schema": "https://vega.github.io/schema/vega-lite/v5.json";,
- "data": {"values": data},
+ "data": {"values": preview_data},
"mark": mark,
}
# Get x_axis and metrics from form_data
- x_axis = form_data.get("x_axis")
+ axis = form_data.get("x_axis")
+ x_axis = get_column_name(axis) if axis else "__timestamp"
Review Comment:
The new axis normalization has no renderer-level coverage: the added preview
tests only use string `x_axis`. A legacy grouped timeseries with no `x_axis`
and wide rows keyed by `__timestamp`, or an SQL adhoc `x_axis` with label
`"Month"`, would lose the x encoding or raise an unhashable-dict error if this
line were reverted, and nothing would fail. Can a test assert
`encoding.x.field` for both cases?
--
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]
