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 c4a6f37207baa0fc7bf34140f16bab9bc355b0b0 Author: sadpandajoe <[email protected]> AuthorDate: Fri Sep 25 01:01:26 2026 +0000 fix(echarts): derive bar width from real grid padding and dataset column metadata Informed revision after a second independent Phase 6 review found the prior round's fixes for both majors were each still incomplete: - Bar width was still derived from a flat per-side constant (width - 2*gridOffsetLeft / height - gridOffsetTop - gridOffsetBottom), ignoring whatever this codebase's real grid-padding computation (getPadding, already used to build echartOptions.grid) actually reserves for a legend, axis title, or other chrome. A narrow chart with a side legend can have far less real plot area than width/height alone suggest (reviewer's own repro: a 250px chart with a left legend rendered a ~5.24px bar for a ~0.69px-wide bucket). getGrainBarMaxWidth no longer computes plot-length internally; both transformProps.ts files now compute it once their real, final padding object is known (after legend-layout iteration, compact-chart clamping, and orientation swapping) and apply it as a post-pass over the already-built series array, since that padding isn't available until after series are first constructed. - The x-axis coltype-mismatch coercion still fired whenever a time grain was resolved for the chart, regardless of whether that grain had anything to do with the specific x-axis column — so a genuinely numeric x-axis coexisting with an unrelated dashboard-level grain filter would still be wrongly coerced. Cross-references chartProps.datasource.columns (is_dttm/type_generic per column, set once at the dataset level, independent of any single query response's possibly-malformed coltypes) instead of the grain's mere presence — exactly the ticket's real bug shape ("dataset says temporal, this response's coltypes is malformed"), without the previous round's false-positive risk. The x-axis column identifier used for the datasource lookup mirrors getXAxisColumn's own resolution (physical x_axis value, else granularity_sqla) rather than xAxisLabel/xAxisOrig, which resolve to the __timestamp query-response alias for legacy charts, not the real underlying column name. Guard test file: added a heavily-padded (left-legend) bar-width regression test, rebuilt the "stays non-Temporal" case around a genuinely distinct numeric column with its own dataset metadata and an unrelated extraFormData-driven grain (rather than reusing the temporal fixture with a flipped coltype), and added real shape validation to the xAxis type-reading test helper instead of a bare cast. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../src/MixedTimeseries/transformProps.ts | 93 +++++++----- .../src/Timeseries/transformProps.ts | 107 ++++++++------ .../src/Timeseries/transformers.ts | 16 +- .../Bar/sparseSubDailyBarGeometry.test.ts | 163 ++++++++++++++++++--- 4 files changed, 274 insertions(+), 105 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 391ed2c84e9..db4ad6ca124 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -291,16 +291,27 @@ export default function transformProps( const resolvedTimeGrain = formData.extraFormData?.time_grain_sqla ?? timeGrainSqla; - // A resolved time grain only applies to a genuinely temporal x-axis - // column (time_grain_sqla is meaningless otherwise), so trust it over - // `coltypes` only when `coltypes` gave no usable classification at all — + // `coltypes` on the query response can fail to mark the designated x-axis + // column Temporal for reasons unrelated to what the column actually is — // see the matching comment in Timeseries/transformProps.ts for the full - // rationale (in short: a *valid* coltype classification, e.g. a - // genuinely Numeric x-axis, must never be overridden just because an - // unrelated dashboard-level time-grain filter happens to be active). + // rationale. Cross-reference the datasource's own column definition for + // 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. const rawXAxisDataTypeIsUsable = typeof rawXAxisDataType === 'number'; + const rawXAxisColumnName = isPhysicalColumn(chartProps.rawFormData?.x_axis) + ? chartProps.rawFormData.x_axis + : ((chartProps.rawFormData as { granularity_sqla?: string }) + ?.granularity_sqla ?? undefined); + const xAxisDatasourceColumn = datasource.columns?.find( + column => column.column_name === rawXAxisColumnName, + ); + const isDesignatedTemporalColumn = + !!xAxisDatasourceColumn?.is_dttm || + xAxisDatasourceColumn?.type_generic === GenericDataType.Temporal; const xAxisDataType = - !rawXAxisDataTypeIsUsable && resolvedTimeGrain + !rawXAxisDataTypeIsUsable && isDesignatedTemporalColumn ? GenericDataType.Temporal : rawXAxisDataType; const xAxisType = getAxisType( @@ -341,32 +352,6 @@ export default function transformProps( xAxisType, }); - // Size a bar series to its own grain-bucket pixel width instead of a flat - // constant, so a sparse bucket doesn't visually spill into neighboring, - // unpopulated buckets. Only meaningful when a bar series is actually - // rendered — skip the domain scan otherwise. Combines both queries' data - // for the domain estimate, matching the same combined-domain approach the - // x-axis label spacing formatter below already uses. Unlike Timeseries, - // MixedTimeseries has no chart-orientation control (confirmed: no - // `orientation`/`OrientationType` field on its form data, no xAxis/yAxis - // swap anywhere in this file), so the temporal axis always renders along - // `width` here — no horizontal-orientation case to account for. See - // getGrainBarMaxWidth for the rest of the mechanism. - const barMaxWidthPx = - seriesType === EchartsTimeseriesSeriesType.Bar || - seriesTypeB === EchartsTimeseriesSeriesType.Bar - ? getGrainBarMaxWidth( - xAxisType, - resolvedTimeGrain, - [ - rebasedDataA as Record<string, unknown>[], - rebasedDataB as Record<string, unknown>[], - ], - xAxisLabel, - Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0), - ) - : undefined; - const series: SeriesOption[] = []; const resolvedCurrency = resolveAutoCurrency( @@ -621,7 +606,6 @@ export default function transformProps( theme, labelPosition, lineStyle, - barMaxWidthPx, }, ); @@ -730,7 +714,6 @@ export default function transformProps( theme, labelPosition: labelPositionB, lineStyle, - barMaxWidthPx, }, ); @@ -841,6 +824,46 @@ export default function transformProps( xAxisTitleMarginPx, ); + // Size a bar series to its own grain-bucket pixel width instead of a flat + // constant, so a sparse bucket doesn't visually spill into neighboring, + // unpopulated buckets. Computed here — after `chartPadding` (legend, + // axis title, zoomable padding) is fully finalized — and applied as a + // post-pass over the already-built `series`, so the plot-length used + // matches the actual grid area ECharts will render into (a heavily- + // padded chart, e.g. a side legend, genuinely has far less plot area + // than `width` alone suggests) rather than a flat per-side constant. + // Only meaningful when a bar series is actually rendered — skip the + // domain scan otherwise. Combines both queries' data for the domain + // estimate, matching the same combined-domain approach the x-axis label + // spacing formatter below already uses. MixedTimeseries has no + // chart-orientation control (confirmed: no `orientation`/ + // `OrientationType` field on its form data, no xAxis/yAxis swap anywhere + // in this file), so the temporal axis always renders along `width` — + // no horizontal-orientation case to account for. See getGrainBarMaxWidth + // for the domain/grain part of the mechanism. + if ( + seriesType === EchartsTimeseriesSeriesType.Bar || + seriesTypeB === EchartsTimeseriesSeriesType.Bar + ) { + const barMaxWidthPx = getGrainBarMaxWidth( + xAxisType, + resolvedTimeGrain, + [ + rebasedDataA as Record<string, unknown>[], + rebasedDataB as Record<string, unknown>[], + ], + xAxisLabel, + Math.max(width - chartPadding.left - chartPadding.right, 0), + ); + if (barMaxWidthPx !== undefined) { + series.forEach(s => { + if (s.type === 'bar') { + (s as { barMaxWidth?: number }).barMaxWidth = barMaxWidthPx; + } + }); + } + } + const { setDataMask = () => {}, onContextMenu } = hooks; const alignTicks = yAxisIndex !== yAxisIndexB; 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 30bd63abc4f..fb2caa25f16 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -489,22 +489,40 @@ export default function transformProps( const resolvedTimeGrain = formData.extraFormData?.time_grain_sqla ?? timeGrainSqla; - // A resolved time grain only applies to a genuinely temporal x-axis - // column (time_grain_sqla is meaningless otherwise), so trust it over - // `coltypes` when `coltypes` gave no usable classification at all for - // this column (a missing entry, or a raw SQL-type string instead of a - // GenericDataType member — neither of which is a valid enum value) — - // without this, that gap silently degrades the axis to Category and - // renders the raw timestamp value as a label instead of a formatted - // date. Deliberately narrower than "any mismatch": a resolved time grain - // can also come from a dashboard-level cross-filter that applies to every - // chart regardless of whether that chart's own x-axis is temporal, so a - // *valid* coltype classification (e.g. a genuinely Numeric x-axis column - // like `price`) is trusted as-is and never overridden, even if some - // unrelated time-grain filter happens to be active. + // `coltypes` on the query response can fail to mark the designated x-axis + // column Temporal for reasons unrelated to what the column actually is (a + // missing entry, or a raw SQL-type string instead of a GenericDataType + // member) — the ticket's actual bug shape is exactly this: the dataset's + // own column metadata correctly says the column is temporal, but that + // particular query response's `coltypes` is malformed. Cross-reference + // the datasource's own column definition for the x-axis column + // (`is_dttm`/`type_generic`, set once at the dataset/schema level, + // independent of any given query response) instead of a resolved time + // grain: a chart's x-axis column identity is fixed regardless of which + // filters happen to be active, whereas a resolved `time_grain_sqla` can + // 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). const rawXAxisDataTypeIsUsable = typeof rawXAxisDataType === 'number'; + const rawXAxisColumnName = isPhysicalColumn(chartProps.rawFormData?.x_axis) + ? chartProps.rawFormData.x_axis + : ((chartProps.rawFormData as { granularity_sqla?: string }) + ?.granularity_sqla ?? undefined); + const xAxisDatasourceColumn = datasource.columns?.find( + column => column.column_name === rawXAxisColumnName, + ); + const isDesignatedTemporalColumn = + !!xAxisDatasourceColumn?.is_dttm || + xAxisDatasourceColumn?.type_generic === GenericDataType.Temporal; const xAxisDataType = - !rawXAxisDataTypeIsUsable && resolvedTimeGrain + !rawXAxisDataTypeIsUsable && isDesignatedTemporalColumn ? GenericDataType.Temporal : rawXAxisDataType; const xAxisType = getAxisType( @@ -514,34 +532,6 @@ export default function transformProps( seriesType, ); - // Size a bar series to its own grain-bucket pixel width instead of a flat - // constant, so a sparse bucket doesn't visually spill into neighboring, - // unpopulated buckets. Only meaningful when a bar series is actually - // rendered — skip the domain scan otherwise. Horizontal orientation swaps - // the temporal axis onto the chart's height (see the xAxis/yAxis swap - // below), so the plot-length dimension must follow suit; gridOffsetLeft - // (used for the vertical/width case) is specifically the left-grid - // offset and doesn't apply to the height dimension, hence - // gridOffsetTop/gridOffsetBottom instead. See getGrainBarMaxWidth for the - // rest of the mechanism. - const barMaxWidthPx = - seriesType === EchartsTimeseriesSeriesType.Bar - ? getGrainBarMaxWidth( - xAxisType, - resolvedTimeGrain, - [rebasedData as Record<string, unknown>[]], - xAxisLabel, - isHorizontal - ? Math.max( - height - - TIMESERIES_CONSTANTS.gridOffsetTop - - TIMESERIES_CONSTANTS.gridOffsetBottom, - 0, - ) - : Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0), - ) - : undefined; - const [allRawSeries, sortedTotalValues, minPositiveValue] = extractSeries( rebasedData, { @@ -948,7 +938,6 @@ export default function transformProps( hasDimensions: (groupBy?.length ?? 0) > 0, colorByPrimaryAxis, labelPosition, - barMaxWidthPx, }, ); if (transformedSeries) { @@ -1563,6 +1552,38 @@ export default function transformProps( } } + // Size a bar series to its own grain-bucket pixel width instead of a flat + // constant, so a sparse bucket doesn't visually spill into neighboring, + // unpopulated buckets. Computed here — after `padding` is fully finalized + // (legend layout, compact-chart clamping, rotated-label extra padding, + // and the horizontal-orientation swap above have all already run) and + // applied as a post-pass over the already-built `renderedSeries` — so the + // plot-length used matches the *actual* grid area ECharts will render + // into, not a flat per-side constant: a heavily-padded chart (e.g. a + // side legend eating a large share of a narrow chart) genuinely has far + // less plot area than `width`/`height` alone would suggest. Only + // meaningful when a bar series is actually rendered — skip the domain + // scan otherwise. See getGrainBarMaxWidth for the domain/grain part of + // the mechanism. + if (seriesType === EchartsTimeseriesSeriesType.Bar) { + const barMaxWidthPx = getGrainBarMaxWidth( + xAxisType, + resolvedTimeGrain, + [rebasedData as Record<string, unknown>[]], + xAxisLabel, + isHorizontal + ? Math.max(height - padding.top - padding.bottom, 0) + : Math.max(width - padding.left - padding.right, 0), + ); + if (barMaxWidthPx !== undefined) { + renderedSeries.forEach(s => { + if (s.type === 'bar') { + (s as { barMaxWidth?: number }).barMaxWidth = barMaxWidthPx; + } + }); + } + } + const echartOptions: EChartsCoreOption = { useUTC: true, ...(seriesType === EchartsTimeseriesSeriesType.Bar && diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts index 8ce28217ae0..f198c8d5de1 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -423,7 +423,6 @@ export function transformSeries( hasDimensions?: boolean; colorByPrimaryAxis?: boolean; labelPosition?: string; - barMaxWidthPx?: number; }, ): SeriesOption | undefined { const { name, data } = series; @@ -458,7 +457,6 @@ export function transformSeries( theme, colorByPrimaryAxis = false, labelPosition, - barMaxWidthPx, } = opts; const contexts = seriesContexts[name || ''] || []; const hasForecast = @@ -605,13 +603,13 @@ export function transformSeries( ...(colorByPrimaryAxis ? {} : { itemStyle }), // @ts-ignore type: plotType, - // Cap bar width so a sparse/single data point doesn't stretch across - // several (or all of the) neighboring buckets. barMaxWidthPx, when - // provided, is sized to the resolved time grain's own pixel width on - // the axis (see getGrainBarMaxWidth in utils/series.ts); 100 remains - // the fallback for non-temporal axes or when no grain is resolved. - // Bars with many categories auto-size below this cap either way. - ...(plotType === 'bar' ? { barMaxWidth: barMaxWidthPx ?? 100 } : {}), + // Cap bar width so a single data point doesn't stretch across the + // entire chart area. Bars with many categories auto-size below this + // cap. For a sub-daily time grain, transformProps.ts overrides this + // with a grain-derived value once the chart's real grid padding is + // known (see getGrainBarMaxWidth in utils/series.ts) — 100 is the + // fallback for everything else (non-temporal axes, no resolved grain). + ...(plotType === 'bar' ? { barMaxWidth: 100 } : {}), smooth: seriesType === 'smooth', triggerLineEvent: true, // @ts-expect-error 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 00f2b205195..c5f1630ac98 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 @@ -19,6 +19,7 @@ import { ChartProps, ChartDataResponseResult, + Column, SqlaFormData, } from '@superset-ui/core'; import { GenericDataType } from '@apache-superset/core/common'; @@ -34,6 +35,22 @@ import { TIMESERIES_CONSTANTS } from '../../../src/constants'; const HOUR_GRAIN_MS = 3_600_000; // TIMEGRAIN_TO_TIMESTAMP['PT1H'] +// The dataset's own definition of `__timestamp` as the designated temporal +// column — independent of any given query response's (possibly malformed) +// `coltypes` — matching the shape the fix now cross-references. +const DEFAULT_DATASOURCE_COLUMNS: Column[] = [ + { + column_name: 'count', + is_dttm: false, + type_generic: GenericDataType.Numeric, + }, + { + column_name: '__timestamp', + is_dttm: true, + type_generic: GenericDataType.Temporal, + }, +]; + const BASE_FORM_DATA: SqlaFormData = { ...DEFAULT_FORM_DATA, colorScheme: 'bnbColors', @@ -49,10 +66,11 @@ const BASE_FORM_DATA: SqlaFormData = { function buildOptions( width: number, - data: Record<string, number>[], + data: Record<string, unknown>[], overrides: Partial<ChartDataResponseResult> = {}, formDataOverrides: Partial<SqlaFormData> = {}, height = 400, + datasourceColumns: Column[] = DEFAULT_DATASOURCE_COLUMNS, ) { const chartProps = new ChartProps({ width, @@ -67,13 +85,28 @@ function buildOptions( ], formData: { ...BASE_FORM_DATA, ...formDataOverrides }, theme: supersetTheme, + datasource: { columns: datasourceColumns }, }); return transformProps(chartProps as EchartsTimeseriesChartProps) .echartOptions; } -function xAxisType(xAxis: unknown) { +// A bare `as XAxisComponentOption` cast is not real narrowing — a malformed +// or missing xAxis option would silently produce `undefined` and let a +// `.not.toBe('time')` assertion pass for the wrong reason. Validate the +// minimal shape first so a broken option object fails loudly instead. +function xAxisType(xAxis: unknown): unknown { + if ( + typeof xAxis !== 'object' || + xAxis === null || + Array.isArray(xAxis) || + !('type' in xAxis) + ) { + throw new Error( + `expected a single xAxis option object with a "type" property, got: ${JSON.stringify(xAxis)}`, + ); + } return (xAxis as XAxisComponentOption).type; } @@ -117,6 +150,9 @@ test('a sparse hourly bucket (two sparse points) does not render several grain-w [sparseTimestamps.map(__timestamp => ({ __timestamp }))], '__timestamp', ); + // No legend/title chrome in this fixture, so the plot area matches the + // TIMESERIES_CONSTANTS.gridOffsetLeft-only estimate; the padded-chart + // case below (with a left legend) checks the real-padding-aware path. const plotWidthPx = Math.max( width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0, @@ -257,6 +293,62 @@ test('horizontal orientation: bar width is sized against chart height, not width expect(effective).toBeGreaterThan(wrongWidthBasedPxWidth * 2); }); +test('a chart with heavily-reserved grid space (a left legend on a narrow chart) sizes the bar against the real plot area, not width alone', () => { + // The grain-to-pixel computation must derive the plot length from this + // codebase's own real grid-padding computation (getPadding, reused via + // the `padding` object Timeseries/transformProps.ts already builds for + // the chart's actual grid), not a flat per-side constant — a narrow + // chart with a left-side legend genuinely has far less plot width than + // `width` alone suggests, confirmed independently via an ECharts SSR + // render (a 250px-wide chart with a left legend rendered a real bar + // around ~5.24px while one hourly bucket actually occupied ~0.69px; a + // fix using a flat gridOffsetLeft guess instead of the real legend-aware + // padding would compute a cap many times too loose there). + const width = 250; + const sparseTimestamps = [ + Date.UTC(2024, 0, 1, 1, 0, 0), + Date.UTC(2024, 0, 1, 23, 0, 0), + ]; + const { series, grid } = buildOptions( + width, + sparseTimestamps.map(__timestamp => ({ count: 1, __timestamp })), + {}, + { + showLegend: true, + legendOrientation: 'left', + groupby: ['count'], + }, + ); + const [barSeries] = series as BarSeriesOption[]; + const gridBox = grid as { left?: number; right?: number }; + + const [domainMin, domainMax] = getXAxisDomain( + [sparseTimestamps.map(__timestamp => ({ __timestamp }))], + '__timestamp', + ); + const domainSpanMs = (domainMax as number) - (domainMin as number); + // Sanity check that the fixture actually reserves meaningful legend + // space, so this test can't pass vacuously if the legend didn't render. + const realPlotWidthPx = Math.max( + width - (gridBox.left ?? 0) - (gridBox.right ?? 0), + 0, + ); + expect(realPlotWidthPx).toBeLessThan( + width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, + ); + + const correctGrainPxWidth = (HOUR_GRAIN_MS / domainSpanMs) * realPlotWidthPx; + // The value a flat-gridOffsetLeft (legend-blind) computation would have + // produced, to assert the fix isn't still using that estimate. + const flatOffsetPxWidth = + (HOUR_GRAIN_MS / domainSpanMs) * + Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0); + + const effective = effectiveBarPxWidth(barSeries); + expect(effective).toBeLessThanOrEqual(correctGrainPxWidth * 2); + expect(effective).toBeLessThan(flatOffsetPxWidth); +}); + describe('sparse sub-daily bar chart: x-axis mislabels raw epoch values when coltypes does not mark the column Temporal', () => { // getColtypesMapping (utils/series.ts) builds xAxisDataType purely from // colnames[i] -> coltypes[i]; if that lookup doesn't resolve to @@ -276,7 +368,10 @@ describe('sparse sub-daily bar chart: x-axis mislabels raw epoch values when col const data = sparseTimestamps.map(__timestamp => ({ count: 1, __timestamp })); // The decisive invariant across both mismatch shapes below: a genuinely - // temporal x-axis column whose coltype lookup gave no usable + // temporal x-axis column (per the dataset's own `is_dttm`/`type_generic` + // metadata in datasource.columns — the ticket's actual bug shape: the + // dataset says the column is temporal, but this particular query + // response's coltypes is malformed) whose coltype lookup gave no usable // classification at all (missing entry, or a raw SQL-type string that // isn't a GenericDataType member) must still resolve to a `time` axis. test('coltypes shorter than colnames (temporal entry missing)', () => { @@ -293,21 +388,53 @@ describe('sparse sub-daily bar chart: x-axis mislabels raw epoch values when col expect(xAxisType(xAxis)).toBe('time'); }); - // Deliberately NOT covered by the coercion above, and pinned here as a - // regression guard rather than a gap: a coltype lookup that resolves to a - // *valid* GenericDataType member (Numeric here) is a definite - // classification, not a missing one, and is indistinguishable — using - // only the signals available in transformProps.ts — from a genuinely - // non-temporal x-axis column (e.g. `price`) that happens to coexist 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 regardless of whether that chart's own x-axis is temporal). - // Coercing on a valid-but-different classification would wrongly turn - // that unrelated numeric chart into a time axis, so it must not. - test('a coltype-confirmed Numeric x-axis stays non-Temporal even when an unrelated time grain is resolved', () => { - const { xAxis } = buildOptions(800, data, { - coltypes: [GenericDataType.Numeric, GenericDataType.Numeric], - }); + // 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', () => { + const priceDatasourceColumns: Column[] = [ + { + column_name: 'price', + is_dttm: false, + type_generic: GenericDataType.Numeric, + }, + // The dataset has its own, unrelated temporal column — present to + // make the fixture realistic (a dataset with a numeric x-axis chart + // can still have datetime columns other charts/filters use), not + // referenced by this chart's own x-axis. + { + column_name: 'order_date', + is_dttm: true, + type_generic: GenericDataType.Temporal, + }, + ]; + const { xAxis } = buildOptions( + 800, + [ + { count: 1, price: 10 }, + { count: 1, price: 20 }, + ], + { + colnames: ['count', 'price'], + coltypes: [GenericDataType.Numeric, GenericDataType.Numeric], + }, + { + x_axis: 'price', + granularity_sqla: 'order_date', + // Simulates a dashboard-level cross-filter setting a grain that + // has nothing to do with this chart's own (non-temporal) x-axis. + extraFormData: { time_grain_sqla: 'PT1H' }, + }, + 400, + priceDatasourceColumns, + ); expect(xAxisType(xAxis)).not.toBe('time'); }); });
