rebenitez1802 commented on code in PR #45484:
URL: https://github.com/apache/superset/pull/45484#discussion_r4337104859
##########
superset/mcp_service/chart/sunburst.py:
##########
@@ -39,6 +39,93 @@ class SunburstResultRoles:
secondary_metric: str | None = None
+def grained_column_reference(column: Any) -> str | None:
+ """Return the column name of a grained hierarchy level, else None.
+
+ Recognizes only the BASE_AXIS column-reference shape written by
+ :func:`apply_sunburst_hierarchy_time_grain`; arbitrary Custom SQL columns
+ are not treated as physical references.
+ """
+ if (
+ isinstance(column, Mapping)
+ and column.get("columnType") == "BASE_AXIS"
+ and column.get("expressionType") == "SQL"
+ and column.get("isColumnReference") is True
+ and isinstance(name := column.get("sqlExpression"), str)
+ and name
+ and column.get("label") == name
+ ):
+ return name
+ return None
+
+
+def restore_sunburst_hierarchy_time_grain(form_data: dict[str, Any]) -> None:
+ """Recover the temporal pair from an Explore-saved columns control."""
+ if any(
+ key in form_data
+ for key in (
+ "temporal_column",
+ "granularity_sqla",
+ "time_grain",
+ "time_grain_sqla",
+ )
+ ):
+ return
+ columns = form_data.get("columns")
+ if not isinstance(columns, list):
+ return
+ levels = [
+ (name, column["timeGrain"])
+ for column in columns
+ if (name := grained_column_reference(column)) is not None
+ and column.get("timeGrain") not in (None, "")
+ ]
+ if len(levels) == 1:
+ form_data["granularity_sqla"], form_data["time_grain_sqla"] = levels[0]
+
+
+def apply_sunburst_hierarchy_time_grain(form_data: dict[str, Any]) ->
dict[str, Any]:
+ """Carry the Sunburst time grain on the temporal hierarchy level.
+
+ The Sunburst control panel has no ``granularity_sqla`` or
+ ``time_grain_sqla`` control, and Explore builds its chart-data request from
+ controls only, so those top-level keys never reach the query when a saved
+ chart is opened in Explore. A hierarchy level that is the temporal column
+ therefore carries the grain itself, in the BASE_AXIS column-reference shape
+ the frontend ``normalizeTimeColumn`` emits: the ``columns`` control keeps
it
+ and buildQuery forwards it unchanged. Without a grain, a grained level
+ reverts to the plain column name. The top-level keys are kept because
+ dashboards, validation and update merges read them.
+ """
+ columns = form_data.get("columns")
+ if not isinstance(columns, list):
+ return form_data
+ temporal_column = form_data.get("granularity_sqla")
+ grain = form_data.get("time_grain_sqla")
+ rewritten: list[Any] = []
+ for column in columns:
+ name = column if isinstance(column, str) else
grained_column_reference(column)
+ if name is None or (
+ "granularity_sqla" not in form_data and "time_grain_sqla" not in
form_data
+ ):
+ rewritten.append(column)
+ elif isinstance(grain, str) and grain and name == temporal_column:
+ rewritten.append(
+ {
+ "columnType": "BASE_AXIS",
+ "expressionType": "SQL",
+ "isColumnReference": True,
+ "label": name,
+ "sqlExpression": name,
+ "timeGrain": grain,
+ }
+ )
Review Comment:
π΄ High β Embedded guests are rejected on a grained Sunburst re-saved in
Explore
The guest anti-tamper check `_columns_metrics_modified`
(`superset/security/manager.py:1745-1751`) reduces the requested `BASE_AXIS`
column to `"order_date"`, but compares stored params by exact value. Its
comment says "stored raw params never carry the synthesized `BASE_AXIS` shape",
and this block breaks that. MCP-saved charts still pass only because
`granularity_sqla` is in `_STORED_COLUMN_PARAMS`. After an Explore re-save
drops `granularity_sqla` (the state `restore_sunburst_hierarchy_time_grain`
exists to handle), the check returns `modified=True`, so guest-token users get
"cannot modify chart payload". Fix: don't persist the `BASE_AXIS` shape in
saved params. Or apply `_payload_value_identity(..., is_metric=False)` to
stored column values too, but that touches the guest security check and needs a
careful review.
##########
superset/mcp_service/chart/sunburst.py:
##########
@@ -39,6 +39,93 @@ class SunburstResultRoles:
secondary_metric: str | None = None
+def grained_column_reference(column: Any) -> str | None:
+ """Return the column name of a grained hierarchy level, else None.
+
+ Recognizes only the BASE_AXIS column-reference shape written by
+ :func:`apply_sunburst_hierarchy_time_grain`; arbitrary Custom SQL columns
+ are not treated as physical references.
+ """
+ if (
+ isinstance(column, Mapping)
+ and column.get("columnType") == "BASE_AXIS"
+ and column.get("expressionType") == "SQL"
+ and column.get("isColumnReference") is True
+ and isinstance(name := column.get("sqlExpression"), str)
+ and name
+ and column.get("label") == name
+ ):
+ return name
+ return None
+
+
+def restore_sunburst_hierarchy_time_grain(form_data: dict[str, Any]) -> None:
+ """Recover the temporal pair from an Explore-saved columns control."""
+ if any(
+ key in form_data
+ for key in (
+ "temporal_column",
+ "granularity_sqla",
+ "time_grain",
+ "time_grain_sqla",
+ )
+ ):
+ return
+ columns = form_data.get("columns")
+ if not isinstance(columns, list):
+ return
+ levels = [
+ (name, column["timeGrain"])
+ for column in columns
+ if (name := grained_column_reference(column)) is not None
+ and column.get("timeGrain") not in (None, "")
+ ]
+ if len(levels) == 1:
+ form_data["granularity_sqla"], form_data["time_grain_sqla"] = levels[0]
+
+
+def apply_sunburst_hierarchy_time_grain(form_data: dict[str, Any]) ->
dict[str, Any]:
+ """Carry the Sunburst time grain on the temporal hierarchy level.
+
+ The Sunburst control panel has no ``granularity_sqla`` or
+ ``time_grain_sqla`` control, and Explore builds its chart-data request from
+ controls only, so those top-level keys never reach the query when a saved
+ chart is opened in Explore. A hierarchy level that is the temporal column
+ therefore carries the grain itself, in the BASE_AXIS column-reference shape
+ the frontend ``normalizeTimeColumn`` emits: the ``columns`` control keeps
it
+ and buildQuery forwards it unchanged. Without a grain, a grained level
+ reverts to the plain column name. The top-level keys are kept because
+ dashboards, validation and update merges read them.
+ """
+ columns = form_data.get("columns")
+ if not isinstance(columns, list):
+ return form_data
+ temporal_column = form_data.get("granularity_sqla")
+ grain = form_data.get("time_grain_sqla")
+ rewritten: list[Any] = []
+ for column in columns:
+ name = column if isinstance(column, str) else
grained_column_reference(column)
+ if name is None or (
+ "granularity_sqla" not in form_data and "time_grain_sqla" not in
form_data
+ ):
+ rewritten.append(column)
+ elif isinstance(grain, str) and grain and name == temporal_column:
+ rewritten.append(
+ {
+ "columnType": "BASE_AXIS",
+ "expressionType": "SQL",
+ "isColumnReference": True,
+ "label": name,
+ "sqlExpression": name,
+ "timeGrain": grain,
+ }
+ )
+ else:
+ rewritten.append(name)
+ form_data["columns"] = rewritten
Review Comment:
π‘ Medium β Cross-filtering from a grained Sunburst on a semantic view breaks
the targets
`EchartsSunburst.tsx:80` emits `col: columns[treePath.length - 1]`, which is
now an object for the temporal ring. `_validate_filters`
(`semantic_layers/mapper.py:1501`) rejects any dict `col` with "Adhoc columns
are not supported in Semantic View filters", so every target chart on that
semantic view errors. SQL datasets are unaffected.
##########
superset/semantic_layers/mapper.py:
##########
@@ -1320,6 +1318,42 @@ def _get_grain_time_axis_column(
return None
+def _get_grouping_grains(
+ query_object: ValidatedQueryObject,
+ all_dimensions: dict[str, Dimension],
+) -> dict[str, Grain | None]:
+ """Resolve legacy grain defaults and explicit per-column temporal grains.
+
+ BASE_AXIS references retain their own grains when Explore drops top-level
+ time controls. Like SQL datasource queries, column-local grains take
+ precedence over the legacy extras default and identify their own subjects.
+ """
+ axis = _get_grain_time_axis_column(query_object, all_dimensions)
+ default_grain = query_object.extras.get("time_grain_sqla")
+ requested_grains = (
+ {axis: default_grain} if axis is not None and default_grain else {}
+ )
+ dimension_names = set(all_dimensions)
+ for column in query_object.columns:
+ if (
+ isinstance(column, dict)
+ and column.get("columnType") == "BASE_AXIS"
+ and column.get("isColumnReference") is True
+ ):
+ name = _normalize_column(column, dimension_names)
+ requested_grains[name] = column.get("timeGrain")
Review Comment:
π‘ Medium β Mapper now errors on unrelated non-MCP semantic-view charts
This loop takes the `timeGrain` of every `BASE_AXIS` column, including
non-temporal ones. The frontend `normalizeTimeColumn` wraps any `x_axis`,
categorical or not, with `extras.time_grain_sqla`, which defaults to `P1D`. A
semantic-view chart with a categorical x-axis plus a temporal series column, or
legacy `granularity`, worked on master and now raises
`QueryObjectValidationError: The time grain is not supportedβ¦`. SQL datasources
ignore grains on non-datetime columns (`connectors/sqla/models.py:2275`). Fix:
only record column grains for temporal dimensions, using the same `pa.types`
check as `_get_grain_time_axis_column`.
##########
superset/mcp_service/chart/sunburst.py:
##########
@@ -39,6 +39,93 @@ class SunburstResultRoles:
secondary_metric: str | None = None
+def grained_column_reference(column: Any) -> str | None:
+ """Return the column name of a grained hierarchy level, else None.
+
+ Recognizes only the BASE_AXIS column-reference shape written by
+ :func:`apply_sunburst_hierarchy_time_grain`; arbitrary Custom SQL columns
+ are not treated as physical references.
+ """
+ if (
+ isinstance(column, Mapping)
+ and column.get("columnType") == "BASE_AXIS"
+ and column.get("expressionType") == "SQL"
+ and column.get("isColumnReference") is True
+ and isinstance(name := column.get("sqlExpression"), str)
+ and name
+ and column.get("label") == name
+ ):
+ return name
+ return None
+
+
+def restore_sunburst_hierarchy_time_grain(form_data: dict[str, Any]) -> None:
+ """Recover the temporal pair from an Explore-saved columns control."""
+ if any(
+ key in form_data
+ for key in (
+ "temporal_column",
+ "granularity_sqla",
+ "time_grain",
+ "time_grain_sqla",
+ )
+ ):
+ return
+ columns = form_data.get("columns")
+ if not isinstance(columns, list):
+ return
+ levels = [
+ (name, column["timeGrain"])
+ for column in columns
+ if (name := grained_column_reference(column)) is not None
+ and column.get("timeGrain") not in (None, "")
+ ]
+ if len(levels) == 1:
+ form_data["granularity_sqla"], form_data["time_grain_sqla"] = levels[0]
+
+
+def apply_sunburst_hierarchy_time_grain(form_data: dict[str, Any]) ->
dict[str, Any]:
+ """Carry the Sunburst time grain on the temporal hierarchy level.
+
+ The Sunburst control panel has no ``granularity_sqla`` or
+ ``time_grain_sqla`` control, and Explore builds its chart-data request from
+ controls only, so those top-level keys never reach the query when a saved
+ chart is opened in Explore. A hierarchy level that is the temporal column
+ therefore carries the grain itself, in the BASE_AXIS column-reference shape
+ the frontend ``normalizeTimeColumn`` emits: the ``columns`` control keeps
it
+ and buildQuery forwards it unchanged. Without a grain, a grained level
+ reverts to the plain column name. The top-level keys are kept because
+ dashboards, validation and update merges read them.
+ """
+ columns = form_data.get("columns")
+ if not isinstance(columns, list):
+ return form_data
+ temporal_column = form_data.get("granularity_sqla")
+ grain = form_data.get("time_grain_sqla")
+ rewritten: list[Any] = []
+ for column in columns:
+ name = column if isinstance(column, str) else
grained_column_reference(column)
+ if name is None or (
+ "granularity_sqla" not in form_data and "time_grain_sqla" not in
form_data
+ ):
+ rewritten.append(column)
+ elif isinstance(grain, str) and grain and name == temporal_column:
Review Comment:
π‘ Medium β Dashboard Time Grain native filter no longer affects the grained
level
On dashboards `overrideExtraFormData` only rewrites
`extras.time_grain_sqla`. Sunburst never runs `normalizeTimeColumn` (no
`x_axis`), so the column's saved `timeGrain` wins in `adhoc_column_to_sqla`,
and the semantic mapper also lets column grains win. Before this PR, the plain
column matched `granularity` and took the extras grain
(`models/helpers.py:5070-5080`), so the filter worked. MCP's server path
compensates via `apply_time_grain_to_base_axis`; the frontend has nothing
equivalent.
##########
superset/mcp_service/chart/sunburst.py:
##########
@@ -39,6 +39,93 @@ class SunburstResultRoles:
secondary_metric: str | None = None
+def grained_column_reference(column: Any) -> str | None:
+ """Return the column name of a grained hierarchy level, else None.
+
+ Recognizes only the BASE_AXIS column-reference shape written by
+ :func:`apply_sunburst_hierarchy_time_grain`; arbitrary Custom SQL columns
+ are not treated as physical references.
+ """
+ if (
+ isinstance(column, Mapping)
+ and column.get("columnType") == "BASE_AXIS"
+ and column.get("expressionType") == "SQL"
+ and column.get("isColumnReference") is True
+ and isinstance(name := column.get("sqlExpression"), str)
+ and name
+ and column.get("label") == name
Review Comment:
π’ Low β Grain is invisible and fragile in Explore
The `columns` control has no notion of `timeGrain`. Editing the SQL in
`ColumnSelectPopover.tsx:245` rebuilds `{label, sqlExpression, expressionType}`
and silently drops the grain. Renaming the label makes this `label == name`
check fail, so the MCP round-trip treats the level as arbitrary Custom SQL. On
semantic views the popover treats it as a legacy adhoc value that can't be
re-saved.
##########
superset/mcp_service/chart/compile.py:
##########
@@ -411,20 +411,35 @@ def _classify_as_database_error(exc: BaseException,
dataset_id: int) -> bool:
then falls back to the engine spec's ``extract_errors`` regex patterns β
the same classification the Superset UI uses.
"""
+ from superset.commands.chart.exceptions import (
+ ChartDataQueryFailedError,
+ ChartDataQueryValidationError,
+ )
+
+ # Validation wrappers can carry an annotation chart's database failure.
+ # Only a non-validation query failure may use the engine's generic
fallback.
+ database_exc = None if isinstance(exc, ChartDataQueryValidationError) else
exc
# Direct SQLAlchemy errors (unwrapped or in cause chain)
current: BaseException | None = exc
while current is not None:
if isinstance(current, SQLAlchemyError):
return True
+ if isinstance(current, ChartDataQueryFailedError) and not isinstance(
+ current, ChartDataQueryValidationError
+ ):
+ database_exc = current
current = current.__cause__
+ if database_exc is None:
+ return False
+
# Use the dataset's engine spec to classify (same as the UI)
try:
from superset.daos.dataset import DatasetDAO
dataset = DatasetDAO.find_by_id(dataset_id)
- if dataset and dataset.database and isinstance(exc, Exception):
- errors = dataset.database.db_engine_spec.extract_errors(exc)
+ if dataset and dataset.database and isinstance(database_exc,
Exception):
+ errors =
dataset.database.db_engine_spec.extract_errors(database_exc)
Review Comment:
π’ Low β Annotation error is classified with the main dataset's engine spec
`database_exc` may come from an annotation chart on a different database,
but classification uses
`DatasetDAO.find_by_id(dataset_id).database.db_engine_spec`. The error patterns
can mismatch, and "Unable to connect to the database" then points at the wrong
database. Fix: mention in the message that the failure came from an annotation
layer.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1523,7 +1527,9 @@ def configure_temporal_handling(
if x_is_temporal:
form_data["granularity_sqla"] = form_data.get("x_axis")
if time_grain:
- form_data["time_grain_sqla"] = time_grain
+ form_data["time_grain_sqla"] = (
+ time_grain.value if isinstance(time_grain, TimeGrain) else
time_grain
+ )
Review Comment:
π’ Low β Per-mapper `.value` relies on every future mapper remembering it
All six `TimeGrain` fields are covered today, but the next typed enum field
brings the Jinja `validate_context_types` failure back. Fix: JSON round-trip
`form_data` in `build_query_context_from_form_data`, or add a test that walks
every mapper. Nit: `configure_temporal_handling` is still annotated
`time_grain: str | None` while callers pass `TimeGrain`.
##########
superset/mcp_service/chart/compile.py:
##########
@@ -411,20 +411,35 @@ def _classify_as_database_error(exc: BaseException,
dataset_id: int) -> bool:
then falls back to the engine spec's ``extract_errors`` regex patterns β
the same classification the Superset UI uses.
"""
+ from superset.commands.chart.exceptions import (
+ ChartDataQueryFailedError,
+ ChartDataQueryValidationError,
+ )
+
+ # Validation wrappers can carry an annotation chart's database failure.
+ # Only a non-validation query failure may use the engine's generic
fallback.
+ database_exc = None if isinstance(exc, ChartDataQueryValidationError) else
exc
# Direct SQLAlchemy errors (unwrapped or in cause chain)
current: BaseException | None = exc
while current is not None:
if isinstance(current, SQLAlchemyError):
return True
+ if isinstance(current, ChartDataQueryFailedError) and not isinstance(
+ current, ChartDataQueryValidationError
+ ):
+ database_exc = current
Review Comment:
π’ Low β Typed DB failures inside annotation charts now classify as
`compile_error`
The walk only recognizes `SQLAlchemyError` and non-validation
`ChartDataQueryFailedError`. A `SupersetDBAPIError` raised by
`get_columns_description` (`connectors/sqla/utils.py:225`) during an
annotation's type probe runs outside `query()`'s `try`. It gets wrapped as
`QueryObjectValidationError`, so it ends up as `compile_error`, whereas
master's `extract_errors` fallback returned a connection error. Fix: also treat
`SupersetDBAPIError` subclasses as `database_exc` candidates.
##########
superset/mcp_service/chart/compile.py:
##########
@@ -411,20 +411,35 @@ def _classify_as_database_error(exc: BaseException,
dataset_id: int) -> bool:
then falls back to the engine spec's ``extract_errors`` regex patterns β
the same classification the Superset UI uses.
"""
+ from superset.commands.chart.exceptions import (
+ ChartDataQueryFailedError,
+ ChartDataQueryValidationError,
+ )
+
+ # Validation wrappers can carry an annotation chart's database failure.
+ # Only a non-validation query failure may use the engine's generic
fallback.
+ database_exc = None if isinstance(exc, ChartDataQueryValidationError) else
exc
# Direct SQLAlchemy errors (unwrapped or in cause chain)
current: BaseException | None = exc
while current is not None:
if isinstance(current, SQLAlchemyError):
return True
+ if isinstance(current, ChartDataQueryFailedError) and not isinstance(
+ current, ChartDataQueryValidationError
+ ):
+ database_exc = current
current = current.__cause__
Review Comment:
π’ Low β Cause-chain loop has no cycle guard
`current = current.__cause__` would spin forever on a cyclic chain (e.g.
`raise e from e`). The loop predates this PR and no current path builds a
cycle, but it is cheap to harden. Fix: track a `seen` set of `id(current)`.
##########
tests/unit_tests/mcp_service/chart/test_compile.py:
##########
@@ -1133,3 +1134,121 @@ def
test_compile_chart_returns_structured_error_for_malformed_gantt_form_data(
assert result.error_obj.error_type == "compile_error"
assert "tooltip_columns must contain at most 50 entries" in (result.error
or "")
mock_cmd_cls.assert_not_called()
+
+
[email protected](
+ "message",
+ [
+ "Error in jinja expression in metric expression: "
+ "Unsafe template value for key time_grain: TimeGrain",
+ "Invalid post-processing operation",
+ ],
+)
+def test_compile_chart_validation_failure_is_not_connection_error(
+ mocker: MockerFixture, message: str
+) -> None:
+ """Typed template/processing failures must bypass generic DB
classification."""
+ from superset.db_engine_specs.base import BaseEngineSpec
+ from superset.exceptions import (
+ QueryObjectValidationError,
+ QueryObjectValidationMessage,
+ )
+
+ context = Mock()
+ context.get_payload.return_value = {
+ "queries": [
+ {"error":
QueryObjectValidationMessage(QueryObjectValidationError(message))}
+ ]
+ }
+ mocker.patch(
+
"superset.mcp_service.chart.chart_helpers.build_query_context_from_form_data",
+ return_value=context,
+ )
+ mocker.patch("superset.charts.data.form_data.set_query_context_form_data")
+ dataset = Mock()
+ dataset.database.db_engine_spec = BaseEngineSpec
+ mocker.patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=dataset)
+
+ result = _compile_chart({"metrics": ["count"]}, dataset_id=1)
+
+ assert not result.success
+ assert result.error_obj is not None
+ assert result.error_obj.error_type == "compile_error"
+ assert result.error_obj.error_code == "CHART_COMPILE_FAILED"
+ assert message in result.error_obj.details
+
+
[email protected](
+ ("message", "validation_failure", "expected_error_type"),
+ [
+ (
+ "(psycopg2.OperationalError) connection to server failed: "
+ "Connection refused",
+ False,
+ "database_connection_error",
+ ),
+ (
+ "Unsafe template value for key time_grain: TimeGrain",
+ True,
+ "compile_error",
+ ),
+ ("Invalid post-processing operation", True, "compile_error"),
+ ],
+)
+def test_compile_chart_annotation_failure_preserves_error_classification(
Review Comment:
π’ Low β Annotation "connection" case can't fail
This uses `BaseEngineSpec`, which has no custom error patterns, so every
non-validation message becomes `GENERIC_DB_ENGINE_ERROR`. The psycopg2 text is
cosmetic, and this case also passes on the merge base. Fix: add a
`PostgresEngineSpec` case with a syntax error that should stay `compile_error`,
and a case whose chain holds a `SupersetDBAPIError`.
--
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]
