sadpandajoe commented on code in PR #43770:
URL: https://github.com/apache/superset/pull/43770#discussion_r4226798374
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -366,23 +372,24 @@ def _build_response(
is_builtin: bool,
display_name: str,
query_result: dict[str, Any],
+ data: list[dict[str, Any]],
+ raw_columns: list[str],
+ coltypes: list[int | GenericDataType],
query_duration_ms: int,
warnings: list[str],
temporal_columns: set[str] | None = None,
valid_grains: dict[str, dict[str, str]] | None = None,
) -> GetTableResponse:
"""Format the query result into a GetTableResponse."""
- data = query_result.get("data", [])
- raw_columns = query_result.get("colnames", [])
cache_status = get_cache_status_from_result(
query_result, force_refresh=request.force_refresh
)
-
if not data:
+ columns_meta = format_data_columns(data, raw_columns, coltypes)
Review Comment:
The empty-result branch builds column metadata without the temporal
overrides the non-empty path applies below. For a view with temporal date at
monthly grain, get_table with dimensions=[date], time_grain=P1M and a filter
matching no rows can report the date__Month column as data_type string, while
the same selection with rows reports temporal, so the schema depends on row
count. Should result_temporal_columns be computed before the empty branch and
passed to format_data_columns here too?
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -615,75 +997,443 @@ def require_column(value: Any, field_name: str) -> Any:
start_time = require_column(form_data.get("start_time"), "start_time")
end_time = require_column(form_data.get("end_time"), "end_time")
category = require_column(form_data.get("y_axis"), "y_axis")
-
raw_series = form_data.get("series")
series_columns = (
[require_column(raw_series, "series")] if raw_series is not None else
[]
)
-
- raw_tooltip_columns = form_data.get("tooltip_columns") or []
- raw_tooltip_metrics = form_data.get("tooltip_metrics") or []
- if not isinstance(raw_tooltip_columns, list) or len(raw_tooltip_columns) >
50:
+ raw_tooltips = form_data.get("tooltip_columns") or []
+ raw_metrics = form_data.get("tooltip_metrics") or []
+ if not isinstance(raw_tooltips, list) or len(raw_tooltips) > 50:
raise ValueError("Gantt tooltip_columns must contain at most 50
entries")
- if not isinstance(raw_tooltip_metrics, list) or len(raw_tooltip_metrics) >
50:
+ if not isinstance(raw_metrics, list) or len(raw_metrics) > 50:
raise ValueError("Gantt tooltip_metrics must contain at most 50
entries")
tooltip_columns = [
require_column(column, f"tooltip_columns[{index}]")
- for index, column in enumerate(raw_tooltip_columns)
+ for index, column in enumerate(raw_tooltips)
]
+ orderby = _parse_orderby(form_data.get("order_by_cols"))
+ columns = _dedupe_query_fields(
+ [
+ start_time,
+ end_time,
+ category,
+ *series_columns,
+ *tooltip_columns,
+ *(item[0] for item in orderby),
+ ],
+ _column_label,
+ )
+ return columns, list(raw_metrics), orderby, series_columns
+
+
+def _table_time_offsets(form_data: dict[str, Any], query: dict[str, Any]) ->
list[Any]:
+ """Resolve the Table plugin's custom/inherit comparison offsets."""
+ offsets: list[Any] = []
+ if _time_comparison(form_data, query.get("metrics") or []):
+ for offset in _as_list(form_data.get("time_compare")):
+ if offset == "custom":
+ offset = form_data.get("start_date_offset")
+ elif offset == "inherit":
+ offset = "inherit"
+ if offset is not None and offset not in offsets:
+ offsets.append(offset)
+ extra = form_data.get("extra_form_data")
+ if isinstance(extra, dict):
+ offset = extra.get("time_compare")
+ if offset and offset not in offsets:
+ offsets = [offset]
+ return offsets
+
+
+def _table_totals_metrics(metrics: list[Any], aggregate: Any) -> list[Any]:
+ """Mirror ``getTotalsMetrics`` for Table summary queries."""
+ if aggregate not in {"SUM", "AVG"}:
+ return metrics
+ result: list[Any] = []
+ for metric in metrics:
+ if isinstance(metric, dict) and metric.get("expressionType") ==
"SIMPLE":
+ result.append({**metric, "aggregate": aggregate})
+ else:
+ result.append(metric)
+ return result
- raw_order = form_data.get("order_by_cols") or []
- if not isinstance(raw_order, list) or len(raw_order) > 100:
- raise ValueError("Gantt order_by_cols must contain at most 100
entries")
- orderby: list[list[Any]] = []
- for index, entry in enumerate(raw_order):
- if isinstance(entry, str):
- if len(entry) > 1000:
- raise ValueError(f"Gantt order_by_cols[{index}] is too long")
- try:
- entry = utils_json.loads(entry)
- except (TypeError, ValueError) as ex:
- raise ValueError(
- f"Gantt order_by_cols[{index}] is not valid JSON"
- ) from ex
- if (
- not isinstance(entry, (list, tuple))
- or len(entry) != 2
- or not isinstance(entry[0], str)
- or not entry[0]
- or not isinstance(entry[1], bool)
- ):
- raise ValueError(
- f"Gantt order_by_cols[{index}] must be [column,
ascending_boolean]"
- )
- orderby.append([entry[0], entry[1]])
- columns: list[Any] = []
- seen: set[str] = set()
- for column in (
- start_time,
- end_time,
- category,
- *series_columns,
- *tooltip_columns,
- *(entry[0] for entry in orderby),
+def _temporal_column(column: Any, form_data: dict[str, Any]) -> Any:
+ """Apply the frontend BASE_AXIS wrapper for a physical temporal column."""
+ if not isinstance(column, str) or not form_data.get("time_grain_sqla"):
+ return column
+ lookup = form_data.get("temporal_columns_lookup")
+ if not isinstance(lookup, dict) or not lookup.get(column):
+ return column
+ return {
+ "timeGrain": form_data["time_grain_sqla"],
+ "columnType": "BASE_AXIS",
+ "sqlExpression": column,
+ "label": column,
+ "expressionType": "SQL",
+ **(
+ {"isColumnReference": True}
+ if str(form_data.get("datasource", "")).endswith("__semantic_view")
+ else {}
+ ),
+ }
+
+
+def _normalize_orderby(query: dict[str, Any]) -> None:
+ """Mirror ``normalizeOrderBy`` without dropping independent mixed state."""
+ orderby = query.get("orderby")
+ if (
+ isinstance(orderby, list)
+ and orderby
+ and isinstance(orderby[0], (list, tuple))
+ and len(orderby[0]) == 2
+ and orderby[0][0]
+ and isinstance(orderby[0][1], bool)
):
- key = utils_json.dumps(column, sort_keys=True, default=str)
- if key not in seen:
- seen.add(key)
- columns.append(column)
- return columns, list(raw_tooltip_metrics), orderby, series_columns
+ return
+ query.pop("orderby", None)
+ target = query.get("series_limit_metric") or query.get("legacy_order_by")
+ if target is None:
+ metrics = query.get("metrics") or []
+ target = metrics[0] if metrics else None
+ if target is not None:
+ query["orderby"] = [[target, not query.get("order_desc", True)]]
+
+
+def _time_comparison(form_data: dict[str, Any], metrics: list[Any]) -> bool:
+ return bool(
+ metrics
+ and _as_list(form_data.get("time_compare"))
+ and form_data.get("comparison_type")
+ in {"values", "difference", "percentage", "ratio"}
+ )
-def extract_x_axis_col(form_data: dict[str, Any]) -> str | None:
- """Return the x_axis column name from form_data, or None if not set."""
+def _timeseries_post_processing( # noqa: C901
+ form_data: dict[str, Any],
+ query: dict[str, Any],
+ *,
+ operator_metrics: list[Any] | None = None,
+ complete_timeseries_contract: bool = False,
+) -> list[dict[str, Any]]:
+ """Build the frontend Mixed/Timeseries post-processing contract.
+
+ Timeseries passes its pre-extra-metric QueryObject to every operator, while
+ adding ``extractExtraMetrics`` only to its final query and normal pivot.
+ Mixed passes each layer QueryObject and implements the smaller operator set
+ in its own frontend builder.
+ """
+ metrics = (
+ list(operator_metrics)
+ if operator_metrics is not None
+ else list(query.get("metrics") or [])
+ )
+ metric_labels = [label for metric in metrics if (label :=
_metric_label(metric))]
x_axis = form_data.get("x_axis")
- if isinstance(x_axis, str) and x_axis:
- return x_axis
- if isinstance(x_axis, dict):
- col_name = x_axis.get("column_name")
- return col_name if isinstance(col_name, str) and col_name else None
- return None
+ x_label = (
+ _column_label(x_axis)
+ if x_axis
+ else ("__timestamp" if form_data.get("granularity_sqla") else None)
+ )
+ series = _query_series_columns(query)
+ series_labels = [label for column in series if (label :=
_column_label(column))]
+ offsets = _as_list(form_data.get("time_compare"))
+ comparison = _time_comparison(form_data, metrics)
+ offset_map = {
+ f"{metric}__{offset}": metric for metric in metric_labels for offset
in offsets
+ }
+ pivot_metrics = (
+ [*offset_map.values(), *offset_map.keys()]
+ if comparison
+ else [
+ *metric_labels,
+ *(
+ [
+ label
+ for metric in _timeseries_extra_metrics(form_data)
+ if (label := _metric_label(metric))
+ ]
+ if complete_timeseries_contract
+ else []
+ ),
+ ]
+ )
+ chain: list[dict[str, Any] | None] = []
+ if x_label and pivot_metrics:
+ chain.append(
+ {
+ "operation": "pivot",
+ "options": {
+ "index": [x_label],
+ "columns": series_labels,
+ "aggregates": {
+ metric: {"operator": "mean"} for metric in
pivot_metrics
+ },
+ "drop_missing_columns": not form_data.get(
+ "show_empty_columns", False
+ ),
+ },
+ }
+ )
+ method = form_data.get("resample_method")
+ rule = form_data.get("resample_rule")
+ if method and rule:
+ zero_fill = method == "zerofill"
+ chain.append(
+ {
+ "operation": "resample",
+ "options": {
+ "method": "asfreq" if zero_fill else method,
+ "rule": rule,
+ "fill_value": 0 if zero_fill else None,
+ **(
+ {"fill_time_range": True}
+ if form_data.get("resample_fill_time_range")
+ else {}
+ ),
+ },
+ }
+ )
+ rolling_type = form_data.get("rolling_type")
+ rolling_columns = (
+ [*offset_map.values(), *offset_map.keys()] if comparison else
metric_labels
+ )
+ if rolling_type == "cumsum":
+ chain.append(
+ {
+ "operation": "cum",
+ "options": {
+ "operator": "sum",
+ "columns": {column: column for column in rolling_columns},
+ },
+ }
+ )
+ elif rolling_type in {"sum", "mean", "std"}:
+ chain.append(
+ {
+ "operation": "rolling",
+ "options": {
+ "rolling_type": rolling_type,
+ "window": int(form_data.get("rolling_periods") or 1),
+ "min_periods": int(form_data.get("min_periods") or 0),
+ "columns": {column: column for column in rolling_columns},
+ },
+ }
+ )
+ comparison_type = form_data.get("comparison_type")
+ if comparison and comparison_type != "values":
+ chain.append(
+ {
+ "operation": "compare",
+ "options": {
+ "source_columns": list(offset_map.values()),
+ "compare_columns": list(offset_map.keys()),
+ "compare_type": comparison_type,
+ "drop_original_columns": True,
+ },
+ }
+ )
+ if complete_timeseries_contract and form_data.get("contributionMode"):
+ chain.append(
+ {
+ "operation": "contribution",
+ "options": {
+ "orientation": form_data["contributionMode"],
+ "time_shifts": offsets if comparison else [],
+ },
+ }
+ )
+ if comparison:
+ rename: dict[str, str | None] = {}
+ for shifted, metric in offset_map.items():
+ offset = next(
+ (item for item in offsets if shifted.endswith(f"__{item}")),
None
+ )
+ source = (
+ shifted
+ if comparison_type == "values"
+ else f"{comparison_type}__{metric}__{shifted}"
+ )
+ rename[source] = f"{metric}, {offset}" if len(metrics) > 1 else
offset
+ if rename:
+ chain.append(
+ {
+ "operation": "rename",
+ "options": {"columns": rename, "level": 0, "inplace":
True},
+ }
+ )
+ elif (
+ comparison_type not in {"difference", "percentage", "ratio"}
+ and x_label
+ and len(metrics) == 1
+ and (series_labels or len(offsets) > 1)
+ and form_data.get("truncate_metric") is not None
+ and form_data.get("truncate_metric")
+ ):
+ chain.append(
+ {
+ "operation": "rename",
+ "options": {
+ "columns": {metric_labels[0]: None},
+ "level": 0,
+ "inplace": True,
+ },
+ }
+ )
+ if complete_timeseries_contract:
+ x_axis_sort = form_data.get("x_axis_sort")
+ x_axis_sort_asc = form_data.get("x_axis_sort_asc")
+ sortable_labels = [
+ label
+ for label in [
+ x_label,
+ *(
+ _metric_label(metric)
+ for metric in _as_list(form_data.get("metrics"))
+ ),
+ *(
+ _metric_label(metric)
+ for metric in _timeseries_extra_metrics(form_data)
+ ),
+ ]
+ if label
+ ]
+ if (
+ x_axis_sort is not None
+ and x_axis_sort_asc is not None
+ and x_axis_sort in sortable_labels
+ and not _as_list(form_data.get("groupby"))
+ ):
+ options: dict[str, Any] = {"ascending": x_axis_sort_asc}
+ if x_axis_sort == x_label:
+ options["is_sort_index"] = True
+ else:
+ options["by"] = x_axis_sort
+ chain.append({"operation": "sort", "options": options})
+ chain.append({"operation": "flatten"})
+
+ if complete_timeseries_contract and form_data.get("forecastEnabled") and
x_label:
+ x_axis_grain = (
+ x_axis.get("timeGrain")
+ if isinstance(x_axis, dict)
+ and x_axis.get("expressionType") in {"SIMPLE", "SQL"}
+ else None
+ )
+ time_grain = (
+ x_axis_grain
+ or (query.get("extras") or {}).get("time_grain_sqla")
+ or form_data.get("time_grain_sqla")
+ or "P1D"
+ )
+ chain.append(
+ {
+ "operation": "prophet",
Review Comment:
The Prophet step is now part of the compiled query for forecast charts, but
the XY compile check still limits the query to 2 raw rows. For a saved forecast
chart grouped by region where the two top rows are East and West on the same
day, the pivot yields one timestamp and the forecast step fails, so a valid
rebind or config update returns CHART_COMPILE_FAILED even though the full chart
forecasts fine. Should the compile row limit for forecast-enabled charts be
raised (or the forecast step skipped during compile) so it sees at least two
timestamps?
##########
tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py:
##########
@@ -1410,24 +1627,19 @@ async def test_validation_failure_skips_cache_write(
mock_create_form_data.assert_not_called()
@patch.object(update_chart_preview_module, "_find_dataset")
- @patch.object(update_chart_preview_module, "has_dataset_access",
return_value=False)
- @patch("superset.daos.dataset.DatasetDAO.find_by_id")
@patch(
"superset.mcp_service.commands.create_form_data.MCPCreateFormDataCommand.run"
)
@pytest.mark.asyncio
async def test_dataset_access_denied_short_circuits(
self,
mock_create_form_data,
- mock_find_by_id,
- unused_access_mock,
mock_find_dataset,
mcp_server,
mock_auth,
):
- """has_dataset_access=False → DatasetNotAccessible, no cache write."""
- mock_find_dataset.return_value = _mock_dataset(id=3)
- mock_find_by_id.return_value = _mock_dataset(id=3)
+ """An inaccessible dataset short-circuits before mapping or cache
writes."""
+ mock_find_dataset.return_value = None
Review Comment:
This test used to patch has_dataset_access to False and assert
DatasetNotAccessible; it now mocks _find_dataset to return None, which never
exercises the access check that now lives only inside _find_dataset (the tool's
second check was removed). If _find_dataset stopped calling has_dataset_access,
this test would still pass and an ungranted dataset could reach compilation.
Could this keep _find_dataset real, mock DatasetDAO.find_by_id to return a
dataset with has_dataset_access False, and assert success False with no compile
or cache write?
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -1398,6 +2413,151 @@ def fallback_vega_lite_preview(
return None
+# Extended ISO date/datetime text that the renderer's ``Date.parse`` accepts.
+# Python's ISO parser also accepts basic forms such as ``20250101`` and week
+# dates, which browsers reject, so those must not mark the axis temporal.
+_VEGA_TEMPORAL_TEXT = re.compile(
+ r"\d{4}-\d{2}-\d{2}"
+ r"(?:[T ]\d{2}:\d{2}(?::\d{2}(?:\.\d{1,9})?)?(?:Z|[+-]\d{2}:\d{2})?)?"
+)
+
+
+def _xy_pivot_x_type(values: list[Any]) -> str:
+ """Infer the Vega-Lite x type from every x value, not a character scan.
+
+ The renderer has no column metadata, so text is temporal only when each
+ value is an extended ISO date or datetime the renderer can parse; labels
+ such as ``New York`` and compact dates such as ``20250101`` stay nominal
+ instead of becoming unparseable dates.
+ """
+ present = [value for value in values if value is not None]
+ if not present:
+ return "nominal"
+ if all(
+ isinstance(value, (int, float)) and not isinstance(value, bool)
+ for value in present
+ ):
+ return "quantitative"
+ if all(
+ isinstance(value, (date, datetime))
+ or (
+ isinstance(value, str)
+ and _VEGA_TEMPORAL_TEXT.fullmatch(value) is not None
+ and _gantt_temporal_value(value) is not None
+ )
+ for value in present
+ ):
+ return "temporal"
+ return "nominal"
+
+
+def generate_xy_pivot_vega_lite_preview(
+ data: list[dict[str, Any]], form_data: dict[str, Any], *, mark: str
+) -> VegaLitePreview | None:
+ """Render flattened timeseries pivot columns without dropping grouped
series.
+
+ Folding escaped field paths resolves literal output keys without splitting
+ category values that contain escaped commas. The legend retains each
+ complete metric/category label.
+ Long-form results continue through the generic renderer.
+ """
+ from superset.mcp_service.chart.chart_helpers import _as_list,
_time_comparison
+ from superset.utils.pandas_postprocessing.utils import (
+ escape_separator,
+ FLAT_COLUMN_SEPARATOR,
+ )
+
+ def vega_field(name: str) -> str:
+ """Escape a literal key so Vega-Lite does not read it as a nested
path."""
+ return "".join("\\" + char if char in ".[]\\" else char for char in
name)
+
+ if not data:
+ return None
+ dimensions = [
+ label
+ for column in _as_list(form_data.get("groupby"))
+ if (label := _form_column_label(column))
+ ]
+ if not dimensions or any(label in data[0] for label in dimensions):
+ return None
+ x_axis = _form_column_label(form_data.get("x_axis")) or "__timestamp"
+ if x_axis not in data[0]:
+ return None
+ metric_labels = [
+ label
+ for metric in _as_list(form_data.get("metrics"))
+ if (label := metric_result_label(metric))
+ ]
+ # Chart-data results unescape the flattened column names, while raw
+ # post-processing output keeps escaped separators; match either spelling.
+ series_labels = list(metric_labels)
+ if len(metric_labels) == 1 and _time_comparison(
+ form_data, _as_list(form_data.get("metrics"))
+ ):
+ # A single compared metric renames each shifted series to its bare
+ # offset, so those series carry the offset rather than the metric.
+ series_labels.extend(
+ str(offset) for offset in _as_list(form_data.get("time_compare"))
+ )
+ prefixes = {
+ spelling
+ for label in series_labels
+ for spelling in (label, escape_separator(label))
+ }
+ fields = [
+ field
+ for field in data[0]
+ if field != x_axis
+ and any(
+ field.startswith(prefix + FLAT_COLUMN_SEPARATOR)
+ or field.startswith(prefix + "__")
+ for prefix in prefixes
+ )
+ ]
+ if not fields and len(metric_labels) == 1 and
form_data.get("truncate_metric"):
+ # A single truncated metric drops its label from the pivoted column
+ # names, so every non-x-axis column is one category series.
+ fields = [field for field in data[0] if field != x_axis]
+ if not fields:
+ return None
+ x_type = _xy_pivot_x_type([row.get(x_axis) for row in data])
+ return VegaLitePreview(
+ specification={
+ "$schema": "https://vega.github.io/schema/vega-lite/v5.json";,
+ "data": {"values": data},
+ "transform": [
+ {
+ "fold": [vega_field(field) for field in fields],
+ "as": ["__mcp_xy_series", "__mcp_xy_value"],
+ }
+ ],
+ "mark": mark,
+ "encoding": {
+ "x": {"field": vega_field(x_axis), "type": x_type, "title":
x_axis},
+ "y": {
Review Comment:
This folded-series spec encodes a quantitative y with a nominal color and
never sets stack, so Vega-Lite stacks bars/areas by default. For an unstacked
grouped bar chart (stack null, groupby region) with East=10 and West=20 at one
x value, Explore shows separate bars at 10 and 20 but this preview shows one
stacked bar at 30. Should the y encoding map the chart's saved stack control
(and use stack: null for the unstacked case)?
--
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]
