sadpandajoe commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r3986057386
##########
superset/mcp_service/chart/query_result.py:
##########
@@ -223,3 +223,79 @@ def validate_gauge_query_result(
"""Check Gauge results using the same finite-dial contract as rendering."""
normalized = normalize_gauge_query_result(result, form_data)
return normalized if isinstance(normalized, ChartError) else None
+
+
+def normalize_chart_query_result(result: Any, form_data: Mapping[str, Any]) ->
Any:
+ """Validate chart-specific result contracts before consumers use rows."""
+ if form_data.get("viz_type") != "treemap_v2":
+ return normalize_gauge_query_result(result, form_data)
+ if failure := query_result_failure(result):
+ return failure
+ label = metric_result_label(form_data.get("metric"))
+ hierarchy = form_data.get("groupby")
+ if (
+ not label
+ or not isinstance(hierarchy, list)
+ or not hierarchy
+ or not all(isinstance(column, str) and column for column in hierarchy)
+ or len(set(hierarchy)) != len(hierarchy)
+ or label in hierarchy
+ ):
+ return ChartError(
+ error=(
+ "Treemap requires unique hierarchy columns and a distinct
metric label."
+ ),
+ error_type="InvalidTreemapFormData",
+ )
+ queries = result.get("queries") if isinstance(result, Mapping) else None
+ if not isinstance(queries, list) or len(queries) != 1:
+ return ChartError(
+ error="Treemap requires exactly one query result.",
+ error_type="InvalidTreemapResult",
+ )
+ query = queries[0]
+ rows = query.get("data") if isinstance(query, Mapping) else None
+ if not isinstance(rows, list):
+ return ChartError(
+ error="Treemap query data must be an array of rows.",
+ error_type="InvalidTreemapResult",
+ )
+ if failure := _validate_treemap_rows(rows, hierarchy, label):
+ return failure
+ return result
+
+
+def _validate_treemap_rows(
+ rows: list[Any], hierarchy: list[str], label: str
+) -> ChartError | None:
+ """Require complete hierarchy outputs and finite numeric metric values."""
+ for index, row in enumerate(rows):
+ if not isinstance(row, Mapping) or any(
+ column not in row for column in [*hierarchy, label]
+ ):
+ return ChartError(
+ error=f"Treemap row {index} is missing hierarchy or metric
outputs.",
+ error_type="InvalidTreemapResult",
+ )
+ value = row[label]
+ try:
+ valid = (
+ not isinstance(value, bool)
+ and isinstance(value, (int, float))
Review Comment:
SQL DECIMAL/NUMERIC aggregates can still be `decimal.Decimal` in
`ChartDataCommand` records before JSON conversion, so this `int`/`float` guard
returns `InvalidTreemapMetric` for data the native chart can render. Could this
accept finite `Decimal`/real values while still excluding booleans, with a
Decimal regression?
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1276,6 +1379,16 @@ def map_treemap_config(config: TreemapChartConfig) ->
Dict[str, Any]:
"row_limit": config.row_limit,
"color_scheme": config.color_scheme or "supersetColors",
}
+ for key in _TREEMAP_PRESENTATION_KEYS | {
+ "time_range",
+ "granularity_sqla",
+ "template_params",
+ }:
+ value = getattr(config, key)
+ if value is not None:
+ form_data[key] = (
Review Comment:
This serializes `CurrencyFormat` as `symbol_position`, but the native
Treemap formatter reads `symbolPosition`; an explicit suffix can therefore be
ignored and replaced by the locale default in Explore. Should this call
`CurrencyFormat.to_form_data()` like the other chart mappers?
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -470,6 +475,8 @@ def generate(self) -> VegaLitePreview | ChartError: #
noqa: C901
if result and "queries" in result and len(result["queries"]) > 0:
Review Comment:
These saved Treemap previews query with format fallbacks (50/20/1000 rows)
because `_preview_row_limit()` only honors Gauge, so a chart configured with
`row_limit: 1` can preview categories and geometry absent from the actual chart
while the unsaved path honors 1. Should Treemap use its configured row limit
here too?
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -672,6 +673,104 @@ def _without_generated_gauge_time_filter(
]
+def resolve_treemap_update_config(
+ config: ChartConfig | TreemapChartUpdateConfig,
+ existing: dict[str, Any],
+ *,
+ dataset_rebind: bool = False,
+) -> ChartConfig:
+ """Fill omitted required roles only from an authorized same-dataset
Treemap."""
+ if not isinstance(config, TreemapChartUpdateConfig) or isinstance(
+ config, TreemapChartConfig
+ ):
+ return config
+ values = config.model_dump(exclude_unset=True)
+ if existing.get("viz_type") == "treemap_v2" and not dataset_rebind:
+ for field in ("groupby", "metric"):
+ if field not in config.model_fields_set and field in existing:
+ values[field] = existing[field]
+ resolved = TreemapChartConfig.model_validate(values)
+ resolved.__pydantic_fields_set__ = set(config.model_fields_set)
+ return resolved
+
+
+_TREEMAP_PRESENTATION_KEYS = frozenset(
+ {
+ "color_scheme",
+ "show_labels",
+ "show_upper_labels",
+ "label_type",
+ "label_position",
+ "number_format",
+ "date_format",
+ "currency_format",
+ }
+)
+
+
+def _merge_treemap_filters(
+ existing: dict[str, Any],
+ patch: dict[str, Any],
+ config: TreemapChartConfig,
+ dataset_rebind: bool,
+) -> None:
+ """Separate explicit filter/temporal changes from mapper-generated
defaults."""
+ fields = config.model_fields_set
+ if "temporal_column" not in fields:
+ patch.pop(MCP_DASHBOARD_TIME_FILTER_SUBJECT, None)
+ if "filters" not in fields:
Review Comment:
With `filters` explicit and `temporal_column` omitted, the mapper inserts a
neutral filter for the dataset default time column; this branch removes only
its marker and then overwrites the saved `adhoc_filters`. A Treemap previously
bound to another time column therefore starts querying the default column while
retaining a stale subject marker. Could the existing generated binding be
preserved unless `temporal_column` is explicitly changed?
--
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]
