aminghadersohi commented on code in PR #45571:
URL: https://github.com/apache/superset/pull/45571#discussion_r4351297947
##########
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 in 80118e754d0f087124087fdfc59ebae3e4e2a8e5 (drill payload, factory
and SQL) and e3492ea8b6b73bb03479637bb7e421c314bcd54b (adhoc transport). Both
Sunburst drill actions send raw bucket equality plus `grain`. The factory
retains that predicate alongside a Time Range/Time Column override, and SQL
applies the dataset Hours offset before truncation. Semantic drills select the
provider's grained dimension rather than an exact instant.
Regression test:
`tests/unit_tests/models/test_sunburst_drill_hours_offset.py::test_sunburst_factory_drill_preserves_bucket_with_time_range`
covers samples/full with and without legacy granularity, asserting category
membership as well as counts. The four direct-filter cases fail with the
previous backend; all eight direct/adhoc cases pass at this head.
`tests/unit_tests/models/test_sunburst_drill_hours_offset.py::test_sunburst_drill_grain_matches_ring_bucket_with_hours_offset`
also rejects equal-count/wrong-row results.
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
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
— fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5 (detail already emits
equality plus grain; its shared SQL filter path gets the Hours offset
correction)
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/xAxisDrillByFilter.ts
— not affected: Sunburst drills no longer call the range helper; the unchanged
Timeseries Drill by range contract is outside this Sunburst change
-
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx:52
— not affected: cross-filter payload changes are handled separately in
https://github.com/apache/superset/pull/45598
- 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/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:
Fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5. An explicit Time Column
takes precedence over the lookup, and a hidden control preserves that subject
through Explore reconstruction. Without an explicit subject, all physical
temporal levels still receive the grain. The dashboard Time Column override
follows the same precedence in querying and drills. I also corrected the
description: Explore does persist the lookup in params.
Regression test:
`superset-frontend/src/explore/controlUtils/sunburstTimeGrain.test.ts` —
`Explore preserves an MCP time-column subject in a two-temporal-level
hierarchy` fails before this change and passes after.
`tests/unit_tests/models/test_sunburst_drill_hours_offset.py::test_sunburst_explicit_subject_matches_explore_lookup`
and
`tests/unit_tests/mcp_service/chart/test_sunburst_chart.py::test_mcp_time_column_override_grains_one_hierarchy_level`
reproduce the server-side mismatch (four failures with the old builder, four
passes here).
Siblings checked:
- superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/buildQuery.ts
— fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5
-
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/controlPanel.tsx —
fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/timeGrain.ts —
fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset/common/form_data_query_context.py — fixed in
80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset/mcp_service/chart/chart_helpers.py — not affected: MCP normalizes
dashboard overrides before delegating to the shared builder, covered by the MCP
regression test
- superset/charts/data/api.py — not affected: saved-context GET executes
persisted query_context, rather than recomputing levels from dataset lookup
Final head: `8cc881ab51c29c79d574d7b254933fa7babd3019`.
##########
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:
Fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5 by giving the grain
control a null default while retaining its saved-value initializer. Retained
Time Column subjects also reset when their selected level is removed, without
stashing legacy outside subjects on load
(8cc881ab51c29c79d574d7b254933fa7babd3019).
Regression test: `hiding the Sunburst grain control clears the saved grain`
in `superset-frontend/src/explore/components/Control.test.tsx` renders the
control visible with P1M, hides it, and checks the programmatic reset to null.
It fails with the previous control config and passes here. The existing
no-initial-grain tests still pass.
Additional regression test: `removing an MCP temporal level clears its
hidden Time Column subject` in
`superset-frontend/src/explore/components/Control.test.tsx` fails at
e3492ea8b6b73bb03479637bb7e421c314bcd54b and passes at the final head.
Siblings checked:
-
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/controlPanel.tsx —
fixed in 80118e754d0f087124087fdfc59ebae3e4e2a8e5
- superset-frontend/src/explore/components/Control.tsx — not affected: its
existing reset-on-hide hook works once default is defined
- superset-frontend/src/explore/controlUtils/getControlState.ts — not
affected: a null default does not inject an initial grain
- superset/mcp_service/chart/sunburst.py — not affected: the temporal-state
validator already accepts a null grain
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
— not affected: its resetOnHide=false controls intentionally retain values;
the only saved-value grain initializer found by the sibling search is Sunburst's
-
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/controlPanel.tsx:82
— fixed in 8cc881ab51c29c79d574d7b254933fa7babd3019 (retained-subject reset and
non-stashing)
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]
