rebenitez1802 commented on code in PR #45571:
URL: https://github.com/apache/superset/pull/45571#discussion_r4352996753
##########
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:
🔴 High — Timeseries Drill to detail returns the wrong rows under a dashboard
Time Column override
The new `op != "TEMPORAL_RANGE"` guard applies to every chart type. When a
dashboard Time Column override swaps a Timeseries x-axis from `order_date` to
`ship_date`, `filter_to_remove` becomes `order_date`. The Drill to detail
filter (`EchartsTimeseries.tsx:629-643`) is still `order_date ==` a bucket plus
`grain`, but that bucket now comes from `ship_date`.
- **Master** dropped that filter: too many rows, but the right ones were
included.
- **This PR** keeps it, so it returns the wrong rows. Reproduced through
`QueryContextFactory.create` with the real samples payload, which is
`form_data={"dashboardId": ...}` and so carries no `x_axis` to check:
- August point (3 rows) drilled to 4 rows: two of them shipped in
September, and the Aug 3 shipment was missing;
- September point (3 rows) drilled to 1 row.
The PR's tests only cover `granularity` equal to the filter column, so they
don't catch this.
The suggestion keeps non-range filters only when the Time Column is not
overridden. It drops a strict subset of what master dropped, keeps your
same-column improvement, and leaves the Sunburst cases where the grained level
is the Time Column unchanged. All existing tests pass with it. A test with
`granularity` overridden to another column and `form_data={"dashboardId": 1}`
would lock it in. If you take it, the comment at lines 428-429 needs a small
tweak. Pointing the Timeseries drill filter at the overriding column would be a
better fix, but that can be a follow-up.
```suggestion
if filter["col"] != filter_to_remove
or (
filter["op"] != "TEMPORAL_RANGE"
# Under a different Time Column, a drill filter on
the
# replaced column can hold a bucket of the new one (a
# swapped x-axis), so drop it with the range filter.
and filter_to_remove == granularity
)
```
##########
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:
🔴 High — Existing Explore code now writes this hidden control and silently
turns the grain off
On master Sunburst had no `granularity_sqla` control, so Explore dropped
anything written to it. With this control in place, three existing paths now
store a value:
- **Edit dataset:** `DatasourceControl/index.tsx:266-278` sets it to the
dataset's `main_dttm_col`, or else the first temporal column.
- **Edit chart from a dashboard with a Time Column filter:**
`getFormDataWithDashboardContext.ts:132-134` copies the dashboard's column in.
- **Switching chart type:** `standardizedFormData.ts:61` carries it over.
An explicit subject takes precedence over the lookup (`timeGrain.ts:34-36`
and `_sunburst_temporal_grain`). So a value outside the hierarchy leaves every
ring raw while the Time grain control still shows "Month", and a value naming
one level takes the grain away from the other temporal levels. `extractExtras`
also turns the value into the query's `granularity`, so `_apply_granularity`
moves the chart's time-range filter onto that column.
Repro with the real `getAllControlsState` → `exploreReducer` → `buildQuery`
chain, using `main_dttm_col = created_at`, hierarchy `[order_date, ship_date,
region]` and Month:
- before Edit dataset: `P1M:order_date, P1M:ship_date, raw:region`;
- after it: all three rings raw, and Save persists that.
The reset in `Control.tsx:91-105` only fires on a visible→hidden change, and
a value outside the hierarchy is never visible, so the user can't clear it. On
dashboards, the same precedence means a Time Column filter on a column outside
the hierarchy, a common dashboard-wide setup, turns the grain off while Explore
still applies it.
Fix: don't key the MCP subject on `granularity_sqla`. The simplest option is
to drop this control and go back to the round-1 rule: grain the levels the
lookup marks, plus `granularity_sqla` when it names a level. That reopens the
two-temporal-level MCP/Explore difference from my earlier comment, which is far
less harmful than this. If the MCP subject has to survive Explore, store it
under a Sunburst-owned key that only MCP writes, and honour it only when it
names a hierarchy level.
##########
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:
🔴 High — CI is red: the `semantic_layers` 100% coverage gate fails
`unit-tests` fails at 99.97% with `superset/semantic_layers/mapper.py ...
895->898`
([log](https://github.com/apache/superset/actions/runs/39825051132/job/119986802760)).
No test reaches the timezone-aware branch of the new `_resolve_filter_grain`,
where an epoch-ms drill value on a tz-aware grained dimension keeps its tzinfo.
The test below covers it in the style of the neighbouring drill tests. It uses
a non-UTC zone so that dropping the tz would fail it. With it, `pytest
--cov=superset/semantic_layers/ tests/unit_tests/semantic_layers/
--cov-fail-under=100` reports 100%, and ruff 0.9.7 format and check pass.
```suggestion
map_query_object(query)
def test_semantic_drill_epoch_filter_keeps_timezone_aware_grain(
mocker: MockerFixture,
) -> None:
"""An epoch-ms drill bucket on a tz-aware grain keeps its UTC instant."""
dtype = pa.timestamp("us", tz="America/New_York")
raw = Dimension("order.raw", "order_date", dtype)
monthly = Dimension("order.month", "order_date", dtype,
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": 1785542400000, "grain":
"P1M"}
],
)
[semantic] = map_query_object(query)
# 2026-08-01T00:00Z, not 2026-08-01T00:00 local time.
assert semantic.filters == {
Filter(
PredicateType.WHERE,
monthly,
Operator.EQUALS,
datetime(2026, 7, 31, 20, tzinfo=ZoneInfo("America/New_York")),
)
}
```
--
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]
