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 a59a46b729322a4b22dbce4b3ae4c5fbf867015b Author: sadpandajoe <[email protected]> AuthorDate: Fri Sep 25 00:35:59 2026 +0000 fix(echarts): account for orientation and avoid over-eager Temporal coercion Informed revision after independent Phase 6 review found two real defects in the previous round's fix: - getGrainBarMaxWidth always divided the grain by chart `width`, but a horizontal-orientation bar chart swaps the built xAxis/yAxis option objects, so the temporal axis actually renders along `height` there — confirmed via an ECharts SSR render showing an ~8x-too-loose cap on a wide-and-short horizontal chart. The helper now takes a caller-supplied plotLengthPx instead of computing it internally; Timeseries/ transformProps.ts picks height- or width-based padding depending on isHorizontal. MixedTimeseries has no orientation control at all (verified: no orientation field, no xAxis/yAxis swap), so it keeps the width-based computation unconditionally. - The x-axis coltype-mismatch coercion fired on any mismatch against GenericDataType.Temporal, including when the coltype lookup already gave a different but valid classification (e.g. Numeric). A chart whose x-axis is genuinely non-temporal can still pick up a resolved time grain from an unrelated dashboard-level cross-filter, so that combination must not force the axis to Temporal. The coercion now only fires when the coltype lookup gave no usable classification at all (missing entry, or a non-numeric raw value) — a definite classification, right or wrong, is always trusted. This narrows what the previous round's "mis-typed as Numeric" scenario covers; that specific shape is no longer fixed here, since it's indistinguishable from the false-positive case above using only the signals available in transformProps.ts. Also: only compute the grain-derived bar width when a bar series is actually present (previously ran the domain scan for every temporal chart, including line/area, discarding the result), and replace `xAxis as any` casts in the guard test with a typed narrowing helper. Guard test file updated: the "mis-typed as Numeric" case now asserts the axis stays non-Temporal instead of asserting it becomes time-typed, and a new horizontal-orientation test pins the height-based computation. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../src/MixedTimeseries/transformProps.ts | 47 +++++++---- .../src/Timeseries/transformProps.ts | 53 ++++++++---- .../plugin-chart-echarts/src/utils/series.ts | 15 ++-- .../Bar/sparseSubDailyBarGeometry.test.ts | 95 ++++++++++++++++++---- 4 files changed, 155 insertions(+), 55 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 3d2efe23410..391ed2c84e9 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -293,10 +293,14 @@ export default function transformProps( // 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 the two disagree — see the matching comment in - // Timeseries/transformProps.ts for the full rationale. + // `coltypes` only when `coltypes` gave no usable classification at all — + // 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). + const rawXAxisDataTypeIsUsable = typeof rawXAxisDataType === 'number'; const xAxisDataType = - rawXAxisDataType !== GenericDataType.Temporal && resolvedTimeGrain + !rawXAxisDataTypeIsUsable && resolvedTimeGrain ? GenericDataType.Temporal : rawXAxisDataType; const xAxisType = getAxisType( @@ -339,20 +343,29 @@ 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. Combines both queries' data for the domain - // estimate, matching the same combined-domain approach the x-axis label - // spacing formatter below already uses. See getGrainBarMaxWidth for the - // exact mechanism. - const barMaxWidthPx = getGrainBarMaxWidth( - xAxisType, - resolvedTimeGrain, - [ - rebasedDataA as Record<string, unknown>[], - rebasedDataB as Record<string, unknown>[], - ], - xAxisLabel, - width, - ); + // 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[] = []; 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 d65f4d57df9..30bd63abc4f 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -491,14 +491,20 @@ export default function transformProps( // 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 the two disagree. `coltypes` can fail to mark the - // designated x-axis column Temporal for reasons unrelated to what the - // column actually is (a missing entry, the wrong GenericDataType member, - // or a raw SQL-type string in place of the enum) — without this, that - // gap silently degrades the axis to Category and renders the raw - // timestamp value as a label instead of a formatted date. + // `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. + const rawXAxisDataTypeIsUsable = typeof rawXAxisDataType === 'number'; const xAxisDataType = - rawXAxisDataType !== GenericDataType.Temporal && resolvedTimeGrain + !rawXAxisDataTypeIsUsable && resolvedTimeGrain ? GenericDataType.Temporal : rawXAxisDataType; const xAxisType = getAxisType( @@ -510,14 +516,31 @@ 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. See getGrainBarMaxWidth for the exact mechanism. - const barMaxWidthPx = getGrainBarMaxWidth( - xAxisType, - resolvedTimeGrain, - [rebasedData as Record<string, unknown>[]], - xAxisLabel, - width, - ); + // 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, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts index edd61cf06ac..ad9162f039f 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts @@ -1231,13 +1231,20 @@ export function getMinAndMaxFromBounds( * span. This function does not change what range ECharts decides to * render — only how wide a bar is drawn within whatever range that already * is. + * + * `plotLengthPx` must already be the pixel length of whichever screen + * dimension the temporal axis actually renders along — callers are + * responsible for accounting for orientation (a horizontal bar chart swaps + * the temporal axis onto the chart's vertical/height dimension, not width; + * see the call sites in Timeseries/transformProps.ts and + * MixedTimeseries/transformProps.ts) before calling this. */ export function getGrainBarMaxWidth( xAxisType: AxisType, resolvedTimeGrain: string | undefined, dataRecordArrays: Record<string, unknown>[][], xAxisCol: string, - chartWidth: number, + plotLengthPx: number, ): number | undefined { if (xAxisType !== AxisType.Time || !resolvedTimeGrain) { return undefined; @@ -1255,11 +1262,7 @@ export function getGrainBarMaxWidth( } const domainSpanMs = domainMax > domainMin ? domainMax - domainMin : 2 * ONE_DAY_MS; - const plotWidthPx = Math.max( - chartWidth - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, - 0, - ); - return (grainMs / domainSpanMs) * plotWidthPx; + return (grainMs / domainSpanMs) * Math.max(plotLengthPx, 0); } /** 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 c66cb792bec..00f2b205195 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 @@ -24,6 +24,7 @@ import { import { GenericDataType } from '@apache-superset/core/common'; import { supersetTheme } from '@apache-superset/core/theme'; import type { BarSeriesOption } from 'echarts/charts'; +import type { XAxisComponentOption } from 'echarts'; import transformProps from '../../../src/Timeseries/transformProps'; import { DEFAULT_FORM_DATA } from '../../../src/Timeseries/constants'; import { EchartsTimeseriesSeriesType } from '../../../src/Timeseries/types'; @@ -51,10 +52,11 @@ function buildOptions( data: Record<string, number>[], overrides: Partial<ChartDataResponseResult> = {}, formDataOverrides: Partial<SqlaFormData> = {}, + height = 400, ) { const chartProps = new ChartProps({ width, - height: 400, + height, queriesData: [ { data, @@ -71,6 +73,10 @@ function buildOptions( .echartOptions; } +function xAxisType(xAxis: unknown) { + return (xAxis as XAxisComponentOption).type; +} + // ECharts applies barMinWidth/barMaxWidth as a hard ceiling/floor on top of // whatever barWidth (explicit or auto-computed) would otherwise be used — // "as CSS does" (node_modules/echarts/lib/layout/barGrid.js, calcBarWidthAndOffset: @@ -202,6 +208,55 @@ test('the effective bar-width constraint scales with chart pixel width instead o expect(wide.effective).toBeLessThanOrEqual(wide.correctGrainPxWidth * 2); }); +test('horizontal orientation: bar width is sized against chart height, not width, since the temporal axis renders along height there', () => { + // In horizontal orientation, Timeseries/transformProps.ts swaps the built + // xAxis/yAxis option objects (`[xAxis, yAxis] = [yAxis, xAxis]`), so the + // temporal axis ends up on the chart's *vertical* dimension. A fix that + // still divided the grain by `width` there would compute a cap many + // times too loose (confirmed independently via an ECharts SSR render: a + // 3000x400 horizontal chart with hourly points 22h apart renders an + // actual bar around ~124px, one hour is genuinely ~16px, but dividing by + // `width` alone there yields a ~8x-too-generous cap). Pick a narrow + // width and a tall height so the two dimensions would produce very + // different caps, to decisively catch a width/height mixup either way. + const width = 400; + const height = 3000; + const sparseTimestamps = [ + Date.UTC(2024, 0, 1, 1, 0, 0), + Date.UTC(2024, 0, 1, 23, 0, 0), + ]; + const { series } = buildOptions( + width, + sparseTimestamps.map(__timestamp => ({ count: 1, __timestamp })), + {}, + { orientation: 'horizontal' }, + height, + ); + const [barSeries] = series as BarSeriesOption[]; + + const [domainMin, domainMax] = getXAxisDomain( + [sparseTimestamps.map(__timestamp => ({ __timestamp }))], + '__timestamp', + ); + const domainSpanMs = (domainMax as number) - (domainMin as number); + const plotHeightPx = Math.max( + height - + TIMESERIES_CONSTANTS.gridOffsetTop - + TIMESERIES_CONSTANTS.gridOffsetBottom, + 0, + ); + const correctGrainPxWidth = (HOUR_GRAIN_MS / domainSpanMs) * plotHeightPx; + // The (wrong) width-based value a `width`-only computation would have + // produced, to assert the fix isn't accidentally still using it. + const wrongWidthBasedPxWidth = + (HOUR_GRAIN_MS / domainSpanMs) * + Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0); + + const effective = effectiveBarPxWidth(barSeries); + expect(effective).toBeLessThanOrEqual(correctGrainPxWidth * 2); + expect(effective).toBeGreaterThan(wrongWidthBasedPxWidth * 2); +}); + 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 @@ -220,33 +275,39 @@ describe('sparse sub-daily bar chart: x-axis mislabels raw epoch values when col ]; const data = sparseTimestamps.map(__timestamp => ({ count: 1, __timestamp })); - // The decisive, uniform invariant across all three mismatch shapes below: - // a genuinely temporal x-axis column must resolve to a `time` axis. This - // is checked instead of "the label isn't the raw stringified number" - // because the mis-typed-as-Numeric case formats through - // getNumberFormatter instead of String — producing a SMART_NUMBER - // abbreviation like "1.7T" rather than the literal digit string. That's - // still wrong (an epoch-derived number, not a date), but it would make a - // raw-string-equality check a false negative for that one shape; the - // xAxis.type check catches all three uniformly. + // The decisive invariant across both mismatch shapes below: a genuinely + // temporal x-axis column 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)', () => { const { xAxis } = buildOptions(800, data, { coltypes: [GenericDataType.Numeric], }); - expect((xAxis as any).type).toBe('time'); + expect(xAxisType(xAxis)).toBe('time'); }); - test('coltypes present but the x-axis column is mis-typed as Numeric', () => { + test('coltypes as raw SQL type strings (matches the shape already used by Bar/transformProps.test.ts\'s own fixtures, e.g. "TIMESTAMP")', () => { const { xAxis } = buildOptions(800, data, { - coltypes: [GenericDataType.Numeric, GenericDataType.Numeric], + coltypes: ['BIGINT', 'TIMESTAMP'] as unknown as GenericDataType[], }); - expect((xAxis as any).type).toBe('time'); + expect(xAxisType(xAxis)).toBe('time'); }); - test('coltypes as raw SQL type strings (matches the shape already used by Bar/transformProps.test.ts\'s own fixtures, e.g. "TIMESTAMP")', () => { + // 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: ['BIGINT', 'TIMESTAMP'] as unknown as GenericDataType[], + coltypes: [GenericDataType.Numeric, GenericDataType.Numeric], }); - expect((xAxis as any).type).toBe('time'); + expect(xAxisType(xAxis)).not.toBe('time'); }); });
