aminghadersohi commented on code in PR #45571:
URL: https://github.com/apache/superset/pull/45571#discussion_r4351300514
##########
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:
Fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5. The Sunburst lookup is
guarded by Mapping; the same unguarded pattern in the shared Pivot/Interactive
Pivot adapter receives the guard too.
Regression test:
`tests/unit_tests/models/test_sunburst_drill_hours_offset.py::test_sunburst_malformed_lookup_is_ignored`
covers list, string and integer values (three AttributeErrors before, three
passes after).
`tests/unit_tests/common/test_form_data_query_context.py::test_temporal_lookup_non_mapping_on_pivot_siblings`
reproduces both sibling failures before the guard and passes after.
Siblings checked:
- superset/common/form_data_query_context.py:610 — fixed in
80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset/common/form_data_query_context.py:580 — fixed in
80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset/common/form_data_query_context.py:877 — fixed in
80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset/common/form_data_query_context.py:1652 — fixed in
80118e754d0f087124087fdfc59ebae3e4e2a8e5 (Interactive Pivot calls the shared
guarded helper)
- superset/common/form_data_query_context.py:654 — not affected: already
checks Mapping
- superset/common/form_data_query_context.py:676 — not affected: already
checks Mapping
- superset/mcp_service/chart/sunburst.py:293 — not affected: already checks
Mapping
Final head: `8cc881ab51c29c79d574d7b254933fa7babd3019`.
##########
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:
Fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5 (drill payload, factory
and SQL) and e3492ea8b6b73bb03479637bb7e421c314bcd54b (adhoc transport). The
drill fixture sends both samples and chart-data payloads through
QueryContextFactory.create, with a dashboard time range and with/without legacy
granularity. It checks which categories are returned, not just the row count.
The direct SQL drill tests also use the actual equality-plus-grain payload.
Regression test:
`tests/unit_tests/models/test_sunburst_drill_hours_offset.py::test_sunburst_factory_drill_preserves_bucket_with_time_range`
fails in the four direct cases with the previous backend; all eight
direct/adhoc cases pass here. Reverting just the SQL offset change also makes
the two direct drill cases and the generated-expression test fail.
The actual DrillByModal conversion path also retains the grain: Regression
test: `Drill by retains the temporal bucket grain through adhoc form data` in
`superset-frontend/src/utils/simpleFilterToAdhoc.test.ts` fails before the
adapter change and passes after.
`tests/unit_tests/common/test_form_data_query_context.py::test_temporal_drill_grain_survives_adhoc_adapters`
reproduces all three backend-adapter losses before and passes after.
Siblings checked:
-
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx
— fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset/common/query_context_factory.py — fixed in
80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset/models/helpers.py — fixed in
80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset/semantic_layers/mapper.py — fixed in
80118e754d0f087124087fdfc59ebae3e4e2a8e5
- tests/unit_tests/models/test_sunburst_drill_hours_offset.py — fixed in
80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset-frontend/src/utils/simpleFilterToAdhoc.ts — fixed in
e3492ea8b6b73bb03479637bb7e421c314bcd54b
- superset-frontend/packages/superset-ui-core/src/query/convertFilter.ts —
fixed in e3492ea8b6b73bb03479637bb7e421c314bcd54b
-
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.ts
— fixed in e3492ea8b6b73bb03479637bb7e421c314bcd54b
- superset/utils/core.py — fixed in e3492ea8b6b73bb03479637bb7e421c314bcd54b
- superset/common/form_data_query_context.py — fixed in
e3492ea8b6b73bb03479637bb7e421c314bcd54b
- superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx — fixed in
e3492ea8b6b73bb03479637bb7e421c314bcd54b (its existing adapter calls preserve
timeGrain)
- superset/mcp_service/chart/schemas.py — not affected: get_chart_info's
filter summary is read-only metadata, not a query reconstruction path
Final head: `8cc881ab51c29c79d574d7b254933fa7babd3019`.
##########
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:
Fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5. The ungrained-level test
checks `op: '=='` on every filter in addition to absence of `grain`.
Regression test: `drill to detail adds no grain to raw temporal or
categorical levels` in
`superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/EchartsSunburst.test.tsx`.
The original raw behavior already passed; I verified the strengthened
assertion fails when the raw fallback is mutated to TEMPORAL_RANGE, and passes
without the mutation. The grained-payload tests in the same file fail against
the previous component and pass here.
Siblings checked:
-
superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/EchartsSunburst.test.tsx
— fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5
-
superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/buildQuery.test.ts
— not affected: checks exact column shapes and applied grains
-
superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/timeGrain.test.ts
— not affected: tests effective-subject precedence and compile-time field
types, not filter absence
Final head: `8cc881ab51c29c79d574d7b254933fa7babd3019`.
--
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]
