aminghadersohi commented on code in PR #45571:
URL: https://github.com/apache/superset/pull/45571#discussion_r4354288866
##########
superset/common/query_context_factory.py:
##########
@@ -433,6 +435,7 @@ def _apply_granularity( # noqa: C901
filter
for filter in query_object.filter
if filter["col"] != filter_to_remove
+ or filter["op"] != "TEMPORAL_RANGE"
Review Comment:
Fixed in 2ddf12b9e7d56620c743af81ba792b78f3ab494f using the suggested
same-column guard. Independent predicates survive when the Time Column is
unchanged; a replacement column drops the stale drill predicate along with the
old range.
Regression test:
`tests/unit_tests/models/test_sunburst_drill_hours_offset.py::test_timeseries_samples_with_swapped_time_column_drops_stale_drill_filter`
sends the minimal dashboard form data through `QueryContextFactory.create`,
for August/September and samples/drill_detail/full. It checks exact returned
timestamps, not just counts. All six cases fail when the factory source is
reverted to d4351c6569 and pass with this commit.
Siblings checked:
- superset/common/query_context_factory.py:436 — fixed in
2ddf12b9e7d56620c743af81ba792b78f3ab494f
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:629
— not affected: its existing original-axis drill payload is unchanged; the
regression pins the factory fallback for a replacement Time Column
- superset/views/datasource/utils.py:143 — not affected: samples/detail use
the same factory with minimal dashboard form data, covered by the regression's
result-type cases
- superset/charts/schemas.py:1760 — not affected: full chart-data requests
use the same factory, covered by the regression's full cases
- superset/mcp_service/chart/chart_helpers.py:1312 — not affected: it
delegates to the shared factory; same-column Sunburst bucket preservation
remains covered by test_sunburst_factory_drill_preserves_bucket_with_time_range
- superset/mcp_service/dashboard_scope.py:799 — not affected: the existing
range-scope refusal remains conservative; its documentation reflects removal of
other filters on a replaced column
Remaining: retargeting the Timeseries frontend drill predicate to the
overriding column is a follow-up; this change restores the suggested backend
fallback.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/controlPanel.tsx:
##########
@@ -39,6 +42,59 @@ 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,
+ default: null,
+ description: t(
+ 'Time grain applied to the temporal levels of the hierarchy.',
+ ),
+ visibility: ({ controls }: ControlPanelsContainerProps) => {
+ const dttmLookup = Object.fromEntries(
+ ensureIsArray(controls?.columns?.options).map(option => [
+ option.column_name,
+ option.is_dttm,
+ ]),
+ );
+ // buildQuery grains only physical temporal levels, so an adhoc
+ // level alone does not show a grain that would not apply.
+ return ensureIsArray(controls?.columns?.value).some(
+ selection =>
+ isPhysicalColumn(selection) && !!dttmLookup[selection],
+ );
+ },
+ },
+ },
+ 'temporal_columns_lookup',
+ {
+ name: 'granularity_sqla',
Review Comment:
Fixed in 2ddf12b9e7d56620c743af81ba792b78f3ab494f. I removed the hidden
granularity_sqla control and restored the lookup-or-legacy-subject rule in both
builders and the shared drill helper. A legacy or dashboard Time Column cannot
disable grains on lookup-marked hierarchy levels.
Regression test: `Explore grains all temporal levels rather than retaining a
hidden Time Column` in
`superset-frontend/src/explore/controlUtils/sunburstTimeGrain.test.ts` fails
with the previous control/helper sources and passes here. `Sunburst does not
retain a hidden Time Column control` in the panel suite also fails before and
passes here. The additional parametrized dataset-edit tests run
getAllControlsState → exploreReducer → buildQuery for both an outside column
and a hierarchy column; dashboard/chart-type carryover tests cover incoming
form data. Those four cases likewise fail with the previous sources and pass
here. Builder, drill-helper and backend-mirror tests cover lookup-marked levels
under Time Column overrides.
Siblings checked:
-
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/controlPanel.tsx —
fixed in 2ddf12b9e7d56620c743af81ba792b78f3ab494f
- superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/timeGrain.ts —
fixed in 2ddf12b9e7d56620c743af81ba792b78f3ab494f
- superset/common/form_data_query_context.py:627 — fixed in
2ddf12b9e7d56620c743af81ba792b78f3ab494f
-
superset-frontend/src/explore/components/controls/DatasourceControl/index.tsx:270
— not affected: its Time Column write is ignored without a Sunburst control,
covered by the reducer regression
-
superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts:132
— not affected: incoming dashboard Time Column is dropped on Sunburst control
reconstruction, covered by the carryover regression
- superset-frontend/src/explore/controlUtils/standardizedFormData.ts:61 —
not affected: carried Time Column is dropped by the same reconstruction,
covered by the carryover regression
- superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/buildQuery.ts
— not affected: it calls the corrected shared helper; override cases are pinned
in buildQuery.test.ts
-
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx
— not affected: it calls the corrected shared helper; outside-hierarchy
overrides are pinned in timeGrain.test.ts
- superset/mcp_service/chart/chart_helpers.py — not affected: it delegates
to the corrected backend builder, covered by
test_mcp_time_column_override_keeps_lookup_marked_grains
Remaining: the two-temporal-level MCP/Explore difference is deliberately
reopened as suggested: a chart without a lookup can grain only its legacy
subject, while Explore grains all lookup-marked levels. The docs and PR
description disclose this. Landing order remains this PR first;
https://github.com/apache/superset/pull/45598 then merges master and adopts
timeGrain.ts.
##########
tests/unit_tests/semantic_layers/mapper_test.py:
##########
@@ -5284,3 +5364,106 @@ def
test_public_provider_completeness_error_becomes_host_error(
get_results(query)
assert excinfo.value.reason == reason
assert excinfo.value.__cause__ is failure
+
+
[email protected]("missing_grain", [False, True])
+def test_sunburst_two_explicit_temporal_levels(
+ mocker: MockerFixture, missing_grain: bool
+) -> None:
+ """Every explicitly grained hierarchy level is selected and validated."""
+ from superset.common.form_data_query_context import (
+ build_query_objects_from_form_data,
+ )
+
+ dimensions = {
+ Dimension("order.raw", "order_date", pa.timestamp("us")),
+ Dimension("order.day", "order_date", pa.timestamp("us"),
grain=Grains.DAY),
+ Dimension("ship.raw", "ship_date", pa.timestamp("us")),
+ }
+ if not missing_grain:
+ dimensions.add(
+ Dimension("ship.day", "ship_date", pa.timestamp("us"),
grain=Grains.DAY)
+ )
+ datasource = mocker.MagicMock()
+ datasource.implementation = AbcOnlyView(dimensions, set())
+ datasource.fetch_values_predicate = None
+ [query] = build_query_objects_from_form_data(
+ {
+ "viz_type": "sunburst_v2",
+ "columns": ["order_date", "ship_date"],
+ "time_grain_sqla": "P1D",
+ "temporal_columns_lookup": {"order_date": True, "ship_date": True},
+ }
+ )
+ query_object = ValidatedQueryObject(
+ datasource=datasource,
+ columns=query["columns"],
+ metrics=[],
+ extras=query["extras"],
+ )
+ if missing_grain:
+ with pytest.raises(QueryObjectValidationError, match="not supported"):
+ validate_query_object(query_object)
+ else:
+ assert validate_query_object(query_object)
+ [semantic] = map_query_object(query_object)
+ assert [(d.name, d.grain) for d in semantic.dimensions] == [
+ ("order_date", Grains.DAY),
+ ("ship_date", Grains.DAY),
+ ]
+
+
[email protected]("group_limit", [False, True])
[email protected]("value", [1785542400000, "2026-08-01T00:00:00"])
+def test_sunburst_semantic_drill_uses_grained_filter_variant(
+ mocker: MockerFixture, value: int | str, group_limit: bool
+) -> None:
+ """A detail/drill filter compares the bucket with its grained dimension."""
+ raw = Dimension("order.raw", "order_date", pa.timestamp("us"))
+ monthly = Dimension(
+ "order.month", "order_date", pa.timestamp("us"), grain=Grains.MONTH
+ )
+ datasource = mocker.MagicMock()
+ datasource.implementation = AbcOnlyView({raw, monthly}, set())
+ datasource.fetch_values_predicate = None
+ query = ValidatedQueryObject(
+ datasource=datasource,
+ columns=["order_date"],
+ metrics=[],
+ filters=[{"col": "order_date", "op": "==", "val": value, "grain":
"P1M"}],
+ series_limit=1 if group_limit else 0,
+ series_columns=["order_date"] if group_limit else [],
+ from_dttm=datetime(2026, 7, 1),
+ to_dttm=datetime(2026, 10, 1),
+ inner_from_dttm=datetime(2026, 8, 1) if group_limit else None,
+ inner_to_dttm=datetime(2026, 9, 1) if group_limit else None,
+ )
+ [semantic] = map_query_object(query)
+ expected = Filter(
+ PredicateType.WHERE, monthly, Operator.EQUALS, datetime(2026, 8, 1)
+ )
+ assert expected in semantic.filters
+ assert {f for f in semantic.filters if f.operator == Operator.EQUALS} ==
{expected}
+ if group_limit:
+ assert semantic.group_limit is not None
+ assert semantic.group_limit.filters is not None
+ assert expected in semantic.group_limit.filters
+ assert semantic.dimensions == [raw]
+
+
+def test_semantic_drill_rejects_unsupported_filter_grain(mocker:
MockerFixture) -> None:
+ """A missing provider grain must not silently become an instant filter."""
+ raw = Dimension("order.raw", "order_date", pa.timestamp("us"))
+ datasource = mocker.MagicMock()
+ datasource.implementation = AbcOnlyView({raw}, set())
+ datasource.fetch_values_predicate = None
+ query = ValidatedQueryObject(
+ datasource=datasource,
+ columns=["order_date"],
+ metrics=[],
+ filters=[
+ {"col": "order_date", "op": "==", "val": "2026-08-01", "grain":
"P1M"}
+ ],
+ )
+ with pytest.raises(QueryObjectValidationError, match="not supported"):
+ map_query_object(query)
Review Comment:
Fixed in 2436f88c49821a36e68ee5257e9e0b748e57bcd4 by adding the suggested
non-UTC timezone branch test.
Regression test:
`tests/unit_tests/semantic_layers/mapper_test.py::test_semantic_drill_epoch_filter_keeps_timezone_aware_grain`
asserts the exact provider dimension and July 31 20:00 America/New_York
instant for the August 1 00:00Z epoch bucket. Omitting this test reproduces
mapper.py 895->898 and the 99.97% coverage failure; including it reaches 100%
with the required semantic_layers coverage command.
Siblings checked:
- tests/unit_tests/semantic_layers/mapper_test.py — fixed in
2436f88c49821a36e68ee5257e9e0b748e57bcd4
- superset/semantic_layers/mapper.py:895 — not affected: the timezone-aware
conversion was correct but untested; no production mapper change is needed
- superset/semantic_layers/mapper.py:609 — not affected: the test exercises
this main-query path through map_query_object
- superset/semantic_layers/mapper.py:1247 — not affected: both main and
group-limit filters call the same conversion;
test_sunburst_semantic_drill_uses_grained_filter_variant covers both with epoch
and string values
Validation: `pytest --cov=superset/semantic_layers/
tests/unit_tests/semantic_layers/ --cov-fail-under=100` passes at the current
head. No live model APIs were used.
##########
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(
+ columns[i],
+ records[i],
+ timeGrain,
+ path,
+ )
+ : undefined;
+ drillToDetailFilters.push(
+ temporalFilter ?? {
+ col: columns[i],
+ op: '==',
+ val: records[i],
+ formattedVal: path,
+ },
+ );
+ if (temporalFilter && i === treePath.length - 1) {
+ drillByFilters.push(temporalFilter);
+ }
Review Comment:
Remaining: Drill by from a categorical child does not carry its temporal
ancestor's bucket. It also does not carry categorical ancestors; this is the
existing last-clicked-level-only Sunburst contract, not a temporal special case
introduced here.
I checked `git show
origin/master:superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx`:
the master Drill by payload contains only `columns[treePath.length - 1]`. At
2ddf12b9e7d56620c743af81ba792b78f3ab494f, `categorical drill-by and root detail
filters retain their behavior` in test/Sunburst/EchartsSunburst.test.tsx pins
that child-only payload, while Drill to detail retains the full path. Carrying
temporal ancestors alone would retain a month but discard a categorical region
ancestor, so I have left ancestor semantics unchanged for a separate maintainer
decision. This thread remains open.
--
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]
