aminghadersohi commented on code in PR #45571:
URL: https://github.com/apache/superset/pull/45571#discussion_r4354588037
##########
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:
Fixed the different-Time-Column regression in
c51044bac6f96fa85048c77c5f13eb44686bbf34. Sunburst hierarchy buckets are
independent of the Time Column; their non-range predicates survive its
replacement. Samples, their total-count query, and CSV/XLSX downloads carry the
source visualization so this does not retain stale Timeseries buckets.
Regression:
`tests/unit_tests/models/test_sunburst_drill_hours_offset.py::test_sunburst_factory_drill_keeps_bucket_on_different_time_column`
executes SQLite queries through `QueryContextFactory.create` for
saved/dashboard `ship_date` Time Columns and samples/detail/full results. All
six cases fail against 2ddf12b9 and pass with this change: exactly the four
August `order_date` rows (or category counts a=2, b=2), not six rows.
`test_timeseries_samples_with_swapped_time_column_drops_stale_drill_filter`
still passes all 12 exact-row cases, with minimal dashboard form data and
explicit Timeseries metadata. Transport regressions also fail before this
change (two backend and four Jest cases).
Siblings checked:
- superset/common/query_context_factory.py:437 — fixed in
c51044bac6f96fa85048c77c5f13eb44686bbf34
- superset/views/datasource/schemas.py:74 — fixed in
c51044bac6f96fa85048c77c5f13eb44686bbf34
- superset/views/datasource/utils.py:120 — fixed in
c51044bac6f96fa85048c77c5f13eb44686bbf34
- superset-frontend/src/components/Chart/DrillDetail/utils.ts:41 — fixed in
c51044bac6f96fa85048c77c5f13eb44686bbf34
- superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:250
— fixed in c51044bac6f96fa85048c77c5f13eb44686bbf34
- superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:248 — not
affected: retains Sunburst viz_type and sends the grain through the existing
adhoc adapter.
- superset/charts/schemas.py:1752 — not affected: forwards form_data to the
shared factory; the full-result regression covers this query shape.
- superset/mcp_service/chart/chart_helpers.py:1312 — not affected: the
saved-chart builder forwards the Sunburst form_data to the same factory.
- superset/common/query_serialization.py:122 — not affected: async
reconstruction preserves form_data and calls the same factory.
- superset/common/tabular_query.py:428 — not affected: chart-neutral raw
queries have no Sunburst source; their default override behavior is unchanged.
- superset/mcp_service/dashboard_scope.py:799 — not affected: conservative
rejection of scoped range replacement is unchanged.
##########
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:
Addressed the missing different-column factory coverage in
c51044bac6f96fa85048c77c5f13eb44686bbf34 with
`tests/unit_tests/models/test_sunburst_drill_hours_offset.py::test_sunburst_factory_drill_keeps_bucket_on_different_time_column`.
It sets Time Column to `ship_date` while drilling August `order_date`, runs
real SQLite queries after `QueryContextFactory.create`, and asserts exact row
membership/category counts for saved/dashboard settings across samples,
drill_detail and full. All six cases fail at 2ddf12b9 and pass here. The
existing Timeseries stale-filter regression also passes all 12 exact-row cases
after adding explicit chart metadata coverage.
Siblings checked:
- superset/common/query_context_factory.py:437 — fixed in
c51044bac6f96fa85048c77c5f13eb44686bbf34
- superset/views/datasource/schemas.py:74 — fixed in
c51044bac6f96fa85048c77c5f13eb44686bbf34
- superset/views/datasource/utils.py:120 — fixed in
c51044bac6f96fa85048c77c5f13eb44686bbf34
- superset-frontend/src/components/Chart/DrillDetail/utils.ts:41 — fixed in
c51044bac6f96fa85048c77c5f13eb44686bbf34
- superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:250
— fixed in c51044bac6f96fa85048c77c5f13eb44686bbf34
- superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:248 — not
affected: retains Sunburst viz_type and sends the grain through the existing
adhoc adapter.
- superset/charts/schemas.py:1752 — not affected: forwards form_data to the
shared factory; the full-result regression covers this query shape.
- superset/mcp_service/chart/chart_helpers.py:1312 — not affected: the
saved-chart builder forwards the Sunburst form_data to the same factory.
- superset/common/query_serialization.py:122 — not affected: async
reconstruction preserves form_data and calls the same factory.
- superset/common/tabular_query.py:428 — not affected: chart-neutral raw
queries have no Sunburst source; their default override behavior is unchanged.
- superset/mcp_service/dashboard_scope.py:799 — not affected: conservative
rejection of scoped range replacement is unchanged.
##########
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:
Commit 2ddf12b9e7d56620c743af81ba792b78f3ab494f removed the original hidden
`granularity_sqla` control fix at the reviewer’s round-2 request in
https://github.com/apache/superset/pull/45571#discussion_r4352996792 . I have
not re-added it.
With `[order_date, ship_date]` and no temporal lookup, an MCP-saved chart
grains only the legacy Time Column; Explore recomputes its lookup and grains
both temporal levels.
`test_sunburst_time_column_does_not_disable_lookup_grains` records that
difference and passes in this run.
Remaining: the MCP/Explore mismatch for multi-temporal hierarchies. This
thread is not fixed.
--
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]
