This is an automated email from the ASF dual-hosted git repository. sadpandajoe pushed a commit to branch fix-subdaily-sparse-bar-xaxis-2 in repository https://gitbox.apache.org/repos/asf/superset.git
commit a2efb409f1a2b270008faa56edf3ca39fe10a28d Author: sadpandajoe <[email protected]> AuthorDate: Fri Sep 25 01:17:29 2026 +0000 fix(echarts): match getXAxisColumn's real precedence for ad-hoc x-axis columns Informed revision after a third independent Phase 6 review found the coltype-mismatch coercion's column-identifier resolution still didn't match this codebase's actual x-axis resolution precedence: The prior round's resolution was `isPhysicalColumn(x_axis) ? x_axis : granularity_sqla` — falling back to granularity_sqla whenever the selected x_axis merely happened to be non-physical (ad-hoc/computed), not only when no x_axis was selected at all. Concrete counter-example: an ad-hoc x_axis (a computed `double_price` expression, genuinely non-temporal) on a chart whose granularity_sqla is a real, unrelated, genuinely temporal column (order_date), with double_price's coltype malformed — the prior logic would fall back to order_date's metadata and wrongly coerce double_price's axis to time. Traced the actual precedence @superset-ui/core's getXAxisColumn uses (query/getXAxis.ts): isXAxisSet (= isQueryFormColumn(x_axis), true for either a physical or a valid ad-hoc column) decides whether x_axis is "the selected axis" at all; granularity_sqla is only ever the fallback for when x_axis isn't set, never merely because the selected one is non-physical. The column-identifier resolution now imports and branches on isXAxisSet directly instead of isPhysicalColumn: when x_axis is set and physical, use its name; when set but ad-hoc, there's no column_name-comparable identifier for a computed expression, so the lookup correctly finds nothing and leaves the axis uncoerced without a special case; only when x_axis isn't set at all does it fall back to granularity_sqla. Guard test: strengthened the existing numeric-x-axis test to use an unusable coltype (its prior usable-coltype fixture short-circuited before ever reaching the datasource-metadata lookup, so it provided no regression protection for that lookup); added a new ad-hoc-x-axis test for this round's exact scenario; both now assert the exact fallback axis type ('category') instead of `.not.toBe('time')`, which would also pass for an axis option missing its type altogether. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../src/MixedTimeseries/transformProps.ts | 13 +++- .../src/Timeseries/transformProps.ts | 31 +++++--- .../Bar/sparseSubDailyBarGeometry.test.ts | 89 +++++++++++++++++++--- 3 files changed, 109 insertions(+), 24 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts index db4ad6ca124..0a8a10ca3c1 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -34,6 +34,7 @@ import { isIntervalAnnotationLayer, isPhysicalColumn, isTimeseriesAnnotationLayer, + isXAxisSet, QueryFormData, QueryFormMetric, resolveAutoCurrency, @@ -298,10 +299,16 @@ export default function transformProps( // the x-axis column (`is_dttm`/`type_generic`) instead of a resolved // time grain, which can come from an unrelated dashboard-level // cross-filter that applies to every chart regardless of whether that - // chart's own x-axis is temporal. + // chart's own x-axis is temporal. The column identifier mirrors + // getXAxisColumn's own precedence (isXAxisSet, true for either a + // physical or a valid ad-hoc x_axis) — see the matching comment in + // Timeseries/transformProps.ts for why an ad-hoc x_axis must not fall + // through to granularity_sqla's metadata. const rawXAxisDataTypeIsUsable = typeof rawXAxisDataType === 'number'; - const rawXAxisColumnName = isPhysicalColumn(chartProps.rawFormData?.x_axis) - ? chartProps.rawFormData.x_axis + const rawXAxisColumnName = isXAxisSet(chartProps.rawFormData) + ? isPhysicalColumn(chartProps.rawFormData.x_axis) + ? chartProps.rawFormData.x_axis + : undefined : ((chartProps.rawFormData as { granularity_sqla?: string }) ?.granularity_sqla ?? undefined); const xAxisDatasourceColumn = datasource.columns?.find( diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index fb2caa25f16..fa2a2cbc984 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -42,6 +42,7 @@ import { isIntervalAnnotationLayer, isPhysicalColumn, isTimeseriesAnnotationLayer, + isXAxisSet, LegendState, resolveAutoCurrency, TimeseriesChartDataResponseResult, @@ -503,16 +504,28 @@ export default function transformProps( // come from an unrelated dashboard-level cross-filter that applies to // every chart on a dashboard, including ones whose x-axis has nothing to // do with time — trusting grain-presence alone would wrongly coerce a - // genuinely non-temporal x-axis (e.g. `price`) in that case. The x-axis - // column identifier used to look it up mirrors getXAxisColumn's own - // resolution: the physical `x_axis` control value when Generic X-Axis is - // in use, else `granularity_sqla` (xAxisLabel/xAxisOrig can't be used - // here — for a legacy, non-Generic-X-Axis chart they resolve to the - // DTTM_ALIAS query-response key, not the real underlying column name - // that datasource.columns indexes by). + // genuinely non-temporal x-axis (e.g. `price`) in that case. + // + // The x-axis column identifier used to look it up mirrors + // getXAxisColumn's own precedence exactly (@superset-ui/core's + // query/getXAxis.ts): `isXAxisSet` (= isQueryFormColumn(x_axis), true for + // EITHER a physical column string OR a valid ad-hoc/computed column) is + // what decides whether `x_axis` is "the selected axis" — granularity_sqla + // is only the fallback when x_axis isn't set at all, not merely whenever + // the selected x_axis happens to be non-physical. An ad-hoc x_axis (e.g. + // a computed `double_price` expression) is still "selected" and must not + // fall through to an unrelated granularity_sqla column's metadata; it + // simply has no datasource.columns entry to look up by name (it isn't a + // physical dataset column at all), so the lookup below correctly finds + // nothing and leaves it uncoerced. (xAxisLabel/xAxisOrig can't be reused + // for this lookup either way — for a legacy, non-Generic-X-Axis chart + // they resolve to the DTTM_ALIAS query-response key, not the real + // underlying column name that datasource.columns indexes by.) const rawXAxisDataTypeIsUsable = typeof rawXAxisDataType === 'number'; - const rawXAxisColumnName = isPhysicalColumn(chartProps.rawFormData?.x_axis) - ? chartProps.rawFormData.x_axis + const rawXAxisColumnName = isXAxisSet(chartProps.rawFormData) + ? isPhysicalColumn(chartProps.rawFormData.x_axis) + ? chartProps.rawFormData.x_axis + : undefined : ((chartProps.rawFormData as { granularity_sqla?: string }) ?.granularity_sqla ?? undefined); const xAxisDatasourceColumn = datasource.columns?.find( diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/sparseSubDailyBarGeometry.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/sparseSubDailyBarGeometry.test.ts index c5f1630ac98..c8427d08839 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/sparseSubDailyBarGeometry.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/sparseSubDailyBarGeometry.test.ts @@ -389,16 +389,27 @@ describe('sparse sub-daily bar chart: x-axis mislabels raw epoch values when col }); // A separate, genuinely numeric column (distinct name and values from - // the temporal fixtures above) as the designated x-axis, correctly - // classified as Numeric by both the query response's coltypes AND the - // dataset's own column metadata — with an unrelated dashboard-level - // time-grain cross-filter (a real, supported Superset feature: such a - // filter can apply to every chart on a dashboard, including ones whose - // x-axis has nothing to do with time) still resolved for this chart. - // Coercing here would wrongly turn this unrelated numeric chart into a - // time axis, so it must not — this is the scenario the datasource-column - // cross-reference exists to rule out. - test('a genuinely numeric x-axis (price) stays non-Temporal even when an unrelated dashboard time-grain filter is resolved', () => { + // the temporal fixtures above) as the designated x-axis, whose own + // dataset column metadata correctly says it's Numeric — with an + // unrelated dashboard-level time-grain cross-filter (a real, supported + // Superset feature: such a filter can apply to every chart on a + // dashboard, including ones whose x-axis has nothing to do with time) + // still resolved for this chart. Coercing here would wrongly turn this + // unrelated numeric chart into a time axis, so it must not — this is the + // scenario the datasource-column cross-reference exists to rule out. + // + // The query response's own coltype for `price` is deliberately + // *unusable* here (a raw SQL-type string, not a GenericDataType member) + // rather than a valid `Numeric` classification: with a usable coltype, + // `rawXAxisDataTypeIsUsable` is already `true` and the function returns + // early without ever reaching the datasource-metadata lookup at all — a + // test built that way would pass identically against the code from + // before the metadata fix existed, providing no actual regression + // protection for it. An unusable coltype forces the function through the + // same "no usable classification, fall back to *something*" branch the + // ticket's real bug takes, so the assertion only passes if the + // datasource lookup itself correctly says `price` isn't temporal. + test('a genuinely numeric x-axis (price) stays non-Temporal even with an unusable coltype and an unrelated dashboard time-grain filter', () => { const priceDatasourceColumns: Column[] = [ { column_name: 'price', @@ -423,7 +434,7 @@ describe('sparse sub-daily bar chart: x-axis mislabels raw epoch values when col ], { colnames: ['count', 'price'], - coltypes: [GenericDataType.Numeric, GenericDataType.Numeric], + coltypes: ['BIGINT'] as unknown as GenericDataType[], // unusable: missing entry }, { x_axis: 'price', @@ -435,6 +446,60 @@ describe('sparse sub-daily bar chart: x-axis mislabels raw epoch values when col 400, priceDatasourceColumns, ); - expect(xAxisType(xAxis)).not.toBe('time'); + // Exact type, not just `.not.toBe('time')`: with seriesType Bar and a + // non-Temporal classification, getAxisType (utils/series.ts) always + // falls through to Category — asserting the real fallback value avoids + // an assertion that would also pass for an axis option that's merely + // missing its `type` altogether. + expect(xAxisType(xAxis)).toBe('category'); + }); + + // An ad-hoc (computed/expression, not physical) x_axis is still "the + // selected axis" per getXAxisColumn's own precedence (isXAxisSet = + // isQueryFormColumn(x_axis), true for either a physical or a valid + // ad-hoc column) — granularity_sqla is only ever a fallback for when + // x_axis isn't set at all. A chart with an ad-hoc, genuinely non-temporal + // x-axis (`double_price`) must not have its axis type decided by an + // unrelated, genuinely temporal `granularity_sqla` column's metadata, + // even though `double_price` itself has no datasource.columns entry to + // confirm it either way (it's a computed expression, not a physical + // column). + test("an ad-hoc, non-physical x-axis does not fall back to granularity_sqla's (unrelated) column metadata", () => { + const { xAxis } = buildOptions( + 800, + [ + { count: 1, double_price: 10 }, + { count: 1, double_price: 20 }, + ], + { + colnames: ['count', 'double_price'], + coltypes: [GenericDataType.Numeric], // unusable: missing entry for double_price + }, + { + x_axis: { + label: 'double_price', + sqlExpression: 'price * 2', + expressionType: 'SQL', + } as unknown as string, + granularity_sqla: 'order_date', + extraFormData: { time_grain_sqla: 'PT1H' }, + }, + 400, + [ + { + column_name: 'count', + is_dttm: false, + type_generic: GenericDataType.Numeric, + }, + // Real, genuinely temporal column — present on the dataset, but + // not what this chart's x-axis actually is. + { + column_name: 'order_date', + is_dttm: true, + type_generic: GenericDataType.Temporal, + }, + ], + ); + expect(xAxisType(xAxis)).toBe('category'); }); });
