aminghadersohi commented on code in PR #43770:
URL: https://github.com/apache/superset/pull/43770#discussion_r4228036517
##########
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:
Fixed. After merging master in 17db085b806ccd0a9ea55857672edfdafac34862, the
compile check uses the shared builder in `compile.py`, which drops
`forecastEnabled` (and `rolling_type`/`rolling_type_b`) from the bounded
sample, so the Prophet step never runs on the two-row compile query; full data
and preview queries keep it. In 91019555303cd0571a604e2b1688826fcfc84f74 I
added `test_compile_sample_skips_forecast_for_grouped_series_on_one_timestamp`,
which runs the real post-processing on East/West rows that share one day. With
the forecast step kept in the sample it fails with "Forecast requires at least
2 data points"; with the fix it passes.
##########
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:
Fixed in 91019555303cd0571a604e2b1688826fcfc84f74. After the merge the fold
lives in `generate_xy_vega_lite_preview`, and it had the same gap. The y
encoding now follows the saved `stack` control: `stack: null` when unstacked,
`zero` for Stack, and `normalize` for Expand.
`test_wide_preview_fold_follows_the_saved_stack_control` covers
None/False/Stack/True/Expand on a grouped bar with East=10 and West=20, and
fails without the change because the encoding has no `stack` key.
##########
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:
Fixed by the merge of master (17db085b806ccd0a9ea55857672edfdafac34862):
`_build_response` now computes `result_temporal_columns` once and passes it to
`format_data_columns` before the empty-result branch, so both paths type the
grain columns the same way. In 91019555303cd0571a604e2b1688826fcfc84f74 I added
`test_get_table_empty_result_keeps_temporal_grain_column_type` (dimensions
`[metric_time]`, `time_grain=P1M`, no rows). It asserts `["temporal",
"string"]`, and it fails with `["string", "string"]` if the overrides are
skipped for empty data.
--
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]
