rebenitez1802 commented on code in PR #45571:
URL: https://github.com/apache/superset/pull/45571#discussion_r4349822181
##########
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx:
##########
@@ -120,26 +123,53 @@ export default function EchartsSunburst(props:
SunburstTransformedProps) {
const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
const drillByFilters: BinaryQueryObjectFilterClause[] = [];
if (columns?.length) {
- treePath.forEach((path, i) =>
- drillToDetailFilters.push({
- col: columns[i],
- op: '==',
- val: records[i],
- formattedVal: path,
- }),
- );
- const val = treePath[treePath.length - 1];
- drillByFilters.push({
- col: columns[treePath.length - 1],
- op: '==',
- val,
- formattedVal: formatSeriesName(val, {
- timeFormatter: getTimeFormatter(formData.dateFormat),
- numberFormatter: getNumberFormatter(formData.numberFormat),
- coltype:
- coltypeMapping?.[getColumnLabel(columns[treePath.length - 1])],
- }),
+ const timeGrain = (formData.extraFormData?.time_grain_sqla ??
+ formData.timeGrainSqla) as TimeGranularity | undefined;
+ treePath.forEach((path, i) => {
+ // Range filters apply the dataset timezone/offset to both bucket
+ // bounds. Equality on a truncated column bypasses that handling.
+ const temporalFilter =
+ timeGrain &&
+ isGrainedLevel(
+ columns[i],
+ formData.temporalColumnsLookup,
+ formData.granularitySqla,
+ )
+ ? getTemporalXAxisDrillByFilter(
Review Comment:
🔴 High — The backend overwrites the drill bucket filter
These filters go out as `{op: 'TEMPORAL_RANGE', val: '<since> : <until>'}`
in both Drill to detail and Drill by. On the samples and chart-data paths,
`QueryContextFactory._apply_filters`
(`superset/common/query_context_factory.py:445-455`) replaces the value of
every `TEMPORAL_RANGE` filter with `query_object.time_range`, and
`_apply_granularity` (`:410-435`) deletes every filter on the granularity
column. So on a dashboard with a native Time Range filter, or for an MCP-saved
chart that carries `granularity_sqla` or `time_range`, drilling into "October
2004" returns rows from the whole range. Sending the drill payload through
`QueryContextFactory` with the fixture from
`test_sunburst_drill_hours_offset.py` returns 6 rows instead of the bucket's 4.
The acceptance runs didn't hit this because that dashboard had only a Time
Grain filter, and form data saved from Explore carries no `time_range`. This
rewrite is why Timeseries uses `{op: '==', grain}` for Drill to detail
(`EchartsTimes
eries.tsx:629-643`, comment at `:478`), and why #45598 uses that shape.
Fix: go back to `{op: '==', val: records[i], grain: timeGrain}` for Drill to
detail (as in 18f0389292), and handle the Hours offset in the backend, since
the grained `==` path builds its expression with `apply_dataset_offset=False`.
Drill by needs the same treatment, or a backend change that stops rewriting
ranges the drill appends. The new paragraph in `exploring-data.mdx:491-493`
isn't accurate under a time range either.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/buildQuery.ts:
##########
@@ -16,14 +16,83 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { buildQueryContext, QueryFormData } from '@superset-ui/core';
+import {
+ AdhocColumn,
+ buildQueryContext,
+ ensureIsArray,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+} from '@superset-ui/core';
+
+// A hierarchy level takes the time grain when it is a physical column that the
+// temporal lookup (or the legacy ``granularity_sqla``) marks temporal.
+export function isGrainedLevel(
+ column: QueryFormColumn,
+ temporalColumnsLookup: Record<string, boolean> | undefined,
+ granularitySqla: string | undefined,
+): boolean {
+ return (
+ isPhysicalColumn(column) &&
+ (!!temporalColumnsLookup?.[column] || granularitySqla === column)
Review Comment:
🟡 Medium — MCP and Explore grain different levels
A level is grained when the lookup marks it or it equals `granularity_sqla`.
MCP-saved Sunbursts have no lookup, so only the `granularity_sqla` column gets
the grain, while Explore recomputes `temporal_columns_lookup` from the dataset
and grains every temporal level. With a hierarchy of `[order_date, ship_date]`,
the MCP output and the dashboard show one grained ring while Explore shows two,
which is the same kind of MCP-vs-Explore mismatch this PR sets out to fix.
Minor: the description says nothing new is written to `params`, but saving from
Explore does store the lookup.
##########
superset/common/form_data_query_context.py:
##########
@@ -604,6 +604,50 @@ def _temporalized_columns(form_data: dict[str, Any],
columns: list[Any]) -> list
return result
+def _sunburst_temporal_grain(form_data: dict[str, Any], query: dict[str, Any])
-> None:
+ """Mirror Sunburst buildQuery: grain only its temporal hierarchy levels.
+
+ Each temporal level becomes the BASE_AXIS column reference
+ ``normalizeTimeColumn`` emits for a physical x-axis. A grain with neither a
+ temporal level nor a legacy ``granularity`` subject is dormant and is
+ dropped, since semantic views reject a grain that has no time column.
+ """
+ extras = dict(query.get("extras") or {})
+ time_grain = extras.get("time_grain_sqla")
+ temporal_lookup = form_data.get("temporal_columns_lookup") or {}
Review Comment:
🟢 Low — Crashes on a lookup that isn't a dict
`form_data.get("temporal_columns_lookup") or {}` followed by `.get(...)`
raises `AttributeError` when the lookup is a list or another non-mapping value.
`_box_temporalized_columns` and `_table_temporalized_columns` in this file
guard with `isinstance(..., Mapping)`, and so does the MCP helper; `Mapping` is
already imported here.
```suggestion
temporal_lookup = form_data.get("temporal_columns_lookup")
if not isinstance(temporal_lookup, Mapping):
temporal_lookup = {}
```
##########
tests/unit_tests/models/test_sunburst_drill_hours_offset.py:
##########
@@ -0,0 +1,206 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""A Sunburst drill on a grained temporal ring returns that ring's rows.
+
+The Sunburst groups a grained temporal level through a ``BASE_AXIS`` column,
+which applies the dataset Hours offset before time grain truncation. Drill to
+detail and Drill by send the clicked bucket as a ``TEMPORAL_RANGE`` filter in
+local (offset) time, so the filter bounds must be shifted back by the offset to
+select the same rows the ring counted.
+"""
+
+from __future__ import annotations
+
+from contextlib import contextmanager
+from datetime import datetime
+from typing import Any
+
+import pytest
+from flask import Flask
+from pytest_mock import MockerFixture
+from sqlalchemy import create_engine
+from sqlalchemy.engine import Engine
+from sqlalchemy.orm.session import Session
+from sqlalchemy.pool import StaticPool
+
+from superset.common.query_object import QueryObject
+from superset.connectors.sqla.models import SqlaTable, TableColumn
+from superset.models.core import Database
+from superset.superset_typing import AdhocColumn, QueryObjectDict
+from superset.utils.core import QueryObjectFilterClause
+
+# Raw (stored) timestamps. With a +5h offset, the first row is locally
+# 2026-08-01 01:00 (August) and the last is locally 2026-09-01 01:00
+# (September), so a filter that ignores the offset moves both across months.
+ROWS = [
+ ("2026-07-31 20:00:00", "a"),
+ ("2026-08-10 12:00:00", "a"),
+ ("2026-08-20 12:00:00", "b"),
+ ("2026-08-31 18:59:59", "b"),
+ ("2026-08-31 20:00:00", "a"),
+ ("2026-09-15 12:00:00", "b"),
+]
+
+# Bucket start (local time) -> exclusive end, the range the frontend sends.
+MONTH_RANGES = {
+ "2026-07-01T00:00:00": "2026-08-01T00:00:00",
+ "2026-08-01T00:00:00": "2026-09-01T00:00:00",
+ "2026-09-01T00:00:00": "2026-10-01T00:00:00",
+}
+
+
[email protected]
+def events_table(mocker: MockerFixture, session: Session) -> SqlaTable:
+ SqlaTable.metadata.create_all(session.get_bind())
+ engine: Engine = create_engine(
+ "sqlite://",
+ connect_args={"check_same_thread": False},
+ poolclass=StaticPool,
+ future=True,
+ )
+ connection = engine.raw_connection()
+ connection.execute("CREATE TABLE events (order_date TIMESTAMP, kind TEXT)")
+ for ts, kind in ROWS:
+ connection.execute("INSERT INTO events VALUES (?, ?)", (ts, kind))
+ connection.commit()
+
+ database = Database(database_name="db", sqlalchemy_uri="sqlite://")
+
+ @contextmanager
+ def mock_get_sqla_engine(catalog=None, schema=None, **kwargs):
+ yield engine
+
+ mocker.patch.object(database, "get_sqla_engine", new=mock_get_sqla_engine)
+ mocker.patch(
+
"superset.connectors.sqla.models.security_manager.get_guest_rls_filters",
+ return_value=[],
+ )
+ mocker.patch(
+ "superset.connectors.sqla.models.security_manager.is_guest_user",
+ return_value=False,
+ )
+ return SqlaTable(
+ database=database,
+ schema=None,
+ table_name="events",
+ offset=5,
+ columns=[
+ TableColumn(column_name="order_date", is_dttm=True,
type="TIMESTAMP"),
+ TableColumn(column_name="kind", type="TEXT"),
+ ],
+ )
+
+
+def _ring_counts(table: SqlaTable) -> dict[str, int]:
+ """Row count per month bucket, grouped the way Sunburst buildQuery does."""
+ query_object = QueryObject(
+ datasource=table,
+ metrics=[{"expressionType": "SQL", "sqlExpression": "COUNT(*)",
"label": "ct"}],
+ columns=[
+ AdhocColumn(
+ timeGrain="P1M",
+ columnType="BASE_AXIS",
+ sqlExpression="order_date",
+ label="order_date",
+ isColumnReference=True,
+ )
+ ],
+ extras={"time_grain_sqla": "P1M"},
+ row_limit=100,
+ )
+ df = table.get_query_result(query_object).df
+ return {
+ ts.strftime("%Y-%m-%dT%H:%M:%S"): int(ct)
+ for ts, ct in zip(df["order_date"], df["ct"], strict=True)
+ }
+
+
+def _drill_filter(since: str, until: str) -> QueryObjectFilterClause:
+ return {"col": "order_date", "op": "TEMPORAL_RANGE", "val": f"{since} :
{until}"}
+
+
+def _drill_to_detail_count(table: SqlaTable, since: str, until: str) -> int:
Review Comment:
🟢 Low — The drill tests skip the path that breaks
These helpers build `QueryObject` directly and call `get_query_result`, so
`QueryContextFactory._process_query_object` never runs: neither
`_apply_filters` nor `_apply_granularity` touches the drill filter, and no test
sets `time_range` or `granularity`. That is how the bucket-filter overwrite
flagged on `EchartsSunburst.tsx` got past the suite. Fix: send the drill
payload through `QueryContextFactory.create`, once with a dashboard
`time_range` and once with `granularity` set.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx:
##########
@@ -120,26 +123,53 @@ export default function EchartsSunburst(props:
SunburstTransformedProps) {
const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
const drillByFilters: BinaryQueryObjectFilterClause[] = [];
if (columns?.length) {
- treePath.forEach((path, i) =>
- drillToDetailFilters.push({
- col: columns[i],
- op: '==',
- val: records[i],
- formattedVal: path,
- }),
- );
- const val = treePath[treePath.length - 1];
- drillByFilters.push({
- col: columns[treePath.length - 1],
- op: '==',
- val,
- formattedVal: formatSeriesName(val, {
- timeFormatter: getTimeFormatter(formData.dateFormat),
- numberFormatter: getNumberFormatter(formData.numberFormat),
- coltype:
- coltypeMapping?.[getColumnLabel(columns[treePath.length - 1])],
- }),
+ const timeGrain = (formData.extraFormData?.time_grain_sqla ??
Review Comment:
🟢 Low — Sunburst form-data fields are untyped
`formData.temporalColumnsLookup`, `granularitySqla` and `timeGrainSqla` are
only typed through `QueryFormData`'s index signature, hence the cast here. Fix:
declare them on `EchartsSunburstFormData` in `types.ts`.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/controlPanel.tsx:
##########
@@ -39,6 +42,36 @@ const config: ControlPanelConfig = {
expanded: true,
controlSetRows: [
['columns'],
+ [
+ {
+ name: 'time_grain_sqla',
+ config: {
+ ...sharedControls.time_grain_sqla,
+ // Keep the saved grain (or none). Unlike the shared control, a
+ // new chart starts without a grain, so temporal levels keep
+ // their raw values until one is chosen.
+ initialValue: (control: ControlState) => control.value,
Review Comment:
🟢 Low — A hidden grain stays in params and fails MCP validation
This control has no `default`, and the reset-on-hide logic in
`Control.tsx:91-99` only runs when `props.default !== undefined`. If a user
picks a grain, removes the temporal level and saves, params keep a
`time_grain_sqla` with no temporal subject, which
`validate_sunburst_temporal_state` (`mcp_service/chart/sunburst.py:323-330`)
rejects as `invalid_temporal_state` on MCP compile and update. Explore itself
still renders, because `buildQuery` drops the unused grain. `resetOnHide`
already defaults to true, and `getControlState` ignores a `null` default for
the initial value, so new charts still start without a grain.
```suggestion
initialValue: (control: ControlState) => control.value,
default: null,
```
##########
superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/EchartsSunburst.test.tsx:
##########
@@ -0,0 +1,216 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { TimeGranularity } from '@superset-ui/core';
+import { render } from '../../../../spec/helpers/testing-library';
+import EchartsSunburst from '../../src/Sunburst/EchartsSunburst';
+import Echart from '../../src/components/Echart';
+import { EventHandlers } from '../../src/types';
+import {
+ EchartsSunburstFormData,
+ SunburstTransformedProps,
+} from '../../src/Sunburst/types';
+
+jest.mock('../../src/components/Echart', () => ({
+ __esModule: true,
+ default: jest.fn(() => null),
+}));
+
+const mockedEchart = jest.mocked(Echart);
+
+// 2003-10-01T00:00:00Z, the start of the month bucket a monthly grain returns.
+const OCTOBER_2003 = 1064966400000;
+
+const rightClickContext = (
+ formData: Record<string, unknown>,
+ records: (number | string | null)[] = [OCTOBER_2003, 'Classic Cars'],
+ names: string[] = ['October', 'Classic Cars'],
+) => {
+ mockedEchart.mockClear();
+ const onContextMenu = jest.fn();
+ const props = {
+ echartOptions: {},
+ formData: {
+ columns: ['order_date', 'product_line'],
+ dateFormat: 'smart_date',
+ numberFormat: 'SMART_NUMBER',
+ vizType: 'sunburst_v2',
+ ...formData,
+ } as unknown as EchartsSunburstFormData,
+ height: 400,
+ width: 400,
+ refs: {},
+ selectedValues: {},
+ setDataMask: jest.fn(),
+ emitCrossFilters: false,
+ coltypeMapping: {},
+ onContextMenu,
+ } as unknown as SunburstTransformedProps;
+
+ render(<EchartsSunburst {...props} />);
+
+ const lastCall = mockedEchart.mock.calls[mockedEchart.mock.calls.length - 1];
+ const { eventHandlers } = lastCall[0] as { eventHandlers: EventHandlers };
+ eventHandlers.contextmenu({
+ data: { records },
+ treePathInfo: [
+ { name: '', dataIndex: 0, value: 100 },
+ ...names.map((name, i) => ({ name, dataIndex: i + 1, value: 40 })),
+ ],
+ event: { stop: jest.fn(), event: { clientX: 10, clientY: 20 } },
+ });
+ return onContextMenu.mock.calls[0][2];
+};
+
+const rightClickOctoberClassicCars = (formData: Record<string, unknown>) =>
+ rightClickContext(formData).drillToDetail;
+
+test('drill to detail on a grained temporal level filters the whole time
bucket', () => {
+ expect(
+ rightClickOctoberClassicCars({
+ timeGrainSqla: 'P1M',
+ temporalColumnsLookup: { order_date: true },
+ }),
+ ).toEqual([
+ {
+ col: 'order_date',
+ op: 'TEMPORAL_RANGE',
+ val: '2003-10-01T00:00:00 : 2003-11-01T00:00:00',
+ formattedVal: 'October',
+ },
+ {
+ col: 'product_line',
+ op: '==',
+ val: 'Classic Cars',
+ formattedVal: 'Classic Cars',
+ },
+ ]);
+});
+
+test('drill to detail uses the dashboard time grain over the saved one', () =>
{
+ const [temporalFilter] = rightClickOctoberClassicCars({
+ timeGrainSqla: 'P1M',
+ granularitySqla: 'order_date',
+ extraFormData: { time_grain_sqla: 'P1Y' },
+ });
+ expect(temporalFilter.val).toBe('2003-10-01T00:00:00 : 2004-10-01T00:00:00');
+});
+
+test('drill to detail adds no grain to raw temporal or categorical levels', ()
=> {
+ const ungrained = [
+ {},
+ { timeGrainSqla: 'P1M' },
+ { temporalColumnsLookup: { order_date: true } },
+ {
+ timeGrainSqla: 'P1M',
+ temporalColumnsLookup: { order_date: true },
+ columns: [
+ { expressionType: 'SQL', sqlExpression: 'order_date', label: 'od' },
+ 'product_line',
+ ],
+ },
+ ];
+ ungrained.forEach(formData =>
+ rightClickOctoberClassicCars(formData).forEach(
+ (filter: Record<string, unknown>) =>
+ expect(filter).not.toHaveProperty('grain'),
Review Comment:
🟢 Low — This assertion no longer tests anything
`not.toHaveProperty('grain')` was written for the `grain` approach in
18f0389292. Since 45642d00 it still passes if an ungrained level wrongly emits
a `TEMPORAL_RANGE` filter. Fix: also assert `op: '=='` on each filter (the
`grain` check becomes meaningful again if Drill to detail moves back to `==` +
`grain`).
--
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]
