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 34982edd9bcaeb26f17a484d0a7009563b06f829 Author: sadpandajoe <[email protected]> AuthorDate: Fri Sep 25 00:06:20 2026 +0000 fix(echarts): size sparse sub-daily bars to their grain, not a flat 100px cap Bar charts with a sub-daily time grain (hourly, 15-minute, etc.) and sparse data rendered a bar for a single grain bucket several hours wider than its own bucket, and could show raw epoch timestamps on the x-axis instead of time-of-day labels. Two independent, verified mechanisms: 1. The only width constraint on a bar series was a flat `barMaxWidth: 100` (px), unrelated to chart width or the resolved time grain. ECharts' own bandwidth fallback for a `time`-type axis with sparse data (either a single point, or points separated by gaps much larger than the grain) computes a width far wider than one grain bucket; the flat cap only partially bounded that. Replace it with a grain-derived width (`getGrainBarMaxWidth` in utils/series.ts): grain length divided by the axis's own visible time span, scaled to the chart's plot width in pixels. For 2+ distinct x-values this uses the real data extent (getXAxisDomain); for exactly one point it uses ECharts' own existing, verified 48-hour default extent for a degenerate single-point time-axis domain (confirmed via a real ECharts SVG/SSR render and by reading calcNiceForTimeScale in echarts/lib/scale/Time.js) rather than introducing a new mechanism for what the visible axis span should be. TIMEGRAIN_TO_TIMESTAMP gained entries for the sub-hour grains (second/minute/5/10/15/30-minute) it was previously missing, which this computation (and the pre-existing minInterval/maxInterval logic) needs for grains finer than an hour. 2. When a query response's `coltypes` doesn't resolve the x-axis column to GenericDataType.Temporal (a missing entry, the wrong GenericDataType member, or a raw SQL-type string in place of the enum), the axis fell back to `category` type and rendered the raw millisecond value as a label. A resolved time grain only makes sense for a genuinely temporal x-axis column, so both Timeseries and MixedTimeseries now trust a resolved `time_grain_sqla` over a non-Temporal coltype lookup for that column specifically, narrow enough that it cannot affect axis-type resolution for a column that isn't this chart's designated time axis. MixedTimeseries duplicates both call sites (its own coltype lookup, and two calls into the shared transformSeries) and got the same two fixes. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../src/MixedTimeseries/transformProps.ts | 44 +++++++++++++++--- .../src/Timeseries/transformProps.ts | 40 +++++++++++++--- .../src/Timeseries/transformers.ts | 12 +++-- .../plugins/plugin-chart-echarts/src/constants.ts | 13 ++++++ .../plugin-chart-echarts/src/utils/series.ts | 53 ++++++++++++++++++++++ .../Bar/sparseSubDailyBarGeometry.test.ts | 38 ++++++++-------- 6 files changed, 165 insertions(+), 35 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 279b85b9aa8..3d2efe23410 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -75,6 +75,7 @@ import { extractTooltipKeys, getAxisType, getColtypesMapping, + getGrainBarMaxWidth, getHorizontalLegendAvailableWidth, getLegendProps, getMinAndMaxFromBounds, @@ -282,7 +283,22 @@ export default function transformProps( getMetricDisplayName(metricsB[0], verboseMap) || ''; const dataTypes = getColtypesMapping(queriesData[0]); - const xAxisDataType = dataTypes?.[xAxisLabel] ?? dataTypes?.[xAxisOrig]; + const rawXAxisDataType = dataTypes?.[xAxisLabel] ?? dataTypes?.[xAxisOrig]; + + // A dashboard-level time grain override (e.g. via a filter or the temporal + // range control) is delivered in extraFormData and should take precedence + // over the chart's own time grain when formatting temporal axes/tooltips. + 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 the two disagree — see the matching comment in + // Timeseries/transformProps.ts for the full rationale. + const xAxisDataType = + rawXAxisDataType !== GenericDataType.Temporal && resolvedTimeGrain + ? GenericDataType.Temporal + : rawXAxisDataType; const xAxisType = getAxisType( stack, xAxisForceCategorical, @@ -320,6 +336,24 @@ export default function transformProps( totalStackedValues: totalStackedValuesB, 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. 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, + ); + const series: SeriesOption[] = []; const resolvedCurrency = resolveAutoCurrency( @@ -574,6 +608,7 @@ export default function transformProps( theme, labelPosition, lineStyle, + barMaxWidthPx, }, ); @@ -682,6 +717,7 @@ export default function transformProps( theme, labelPosition: labelPositionB, lineStyle, + barMaxWidthPx, }, ); @@ -699,12 +735,6 @@ export default function transformProps( if (maxSecondary === undefined) maxSecondary = 1; } - // A dashboard-level time grain override (e.g. via a filter or the temporal - // range control) is delivered in extraFormData and should take precedence - // over the chart's own time grain when formatting temporal axes/tooltips. - const resolvedTimeGrain = - formData.extraFormData?.time_grain_sqla ?? timeGrainSqla; - const tooltipFormatter = xAxisDataType === GenericDataType.Temporal ? getTooltipTimeFormatter(tooltipTimeFormat, resolvedTimeGrain) 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 276db9e6325..d65f4d57df9 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -91,6 +91,7 @@ import { getAreaScaledSymbolSize, getAxisType, getColtypesMapping, + getGrainBarMaxWidth, getHorizontalLegendAvailableWidth, getLegendProps, getMinAndMaxFromBounds, @@ -480,7 +481,26 @@ export default function transformProps( ); const isMultiSeries = groupBy.length || metrics?.length > 1; - const xAxisDataType = dataTypes?.[xAxisLabel] ?? dataTypes?.[xAxisOrig]; + const rawXAxisDataType = dataTypes?.[xAxisLabel] ?? dataTypes?.[xAxisOrig]; + + // A dashboard-level time grain override (e.g. via a filter or the temporal + // range control) is delivered in extraFormData and should take precedence + // over the chart's own time grain when formatting temporal axes/tooltips. + 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 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. + const xAxisDataType = + rawXAxisDataType !== GenericDataType.Temporal && resolvedTimeGrain + ? GenericDataType.Temporal + : rawXAxisDataType; const xAxisType = getAxisType( stack, xAxisForceCategorical, @@ -488,6 +508,17 @@ 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. See getGrainBarMaxWidth for the exact mechanism. + const barMaxWidthPx = getGrainBarMaxWidth( + xAxisType, + resolvedTimeGrain, + [rebasedData as Record<string, unknown>[]], + xAxisLabel, + width, + ); + const [allRawSeries, sortedTotalValues, minPositiveValue] = extractSeries( rebasedData, { @@ -894,6 +925,7 @@ export default function transformProps( hasDimensions: (groupBy?.length ?? 0) > 0, colorByPrimaryAxis, labelPosition, + barMaxWidthPx, }, ); if (transformedSeries) { @@ -1193,12 +1225,6 @@ export default function transformProps( }); } - // A dashboard-level time grain override (e.g. via a filter or the temporal - // range control) is delivered in extraFormData and should take precedence - // over the chart's own time grain when formatting temporal axes/tooltips. - const resolvedTimeGrain = - formData.extraFormData?.time_grain_sqla ?? timeGrainSqla; - const tooltipFormatter = xAxisDataType === GenericDataType.Temporal ? getTooltipTimeFormatter(tooltipTimeFormat, resolvedTimeGrain) 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 55fe2bc1166..8ce28217ae0 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -423,6 +423,7 @@ export function transformSeries( hasDimensions?: boolean; colorByPrimaryAxis?: boolean; labelPosition?: string; + barMaxWidthPx?: number; }, ): SeriesOption | undefined { const { name, data } = series; @@ -457,6 +458,7 @@ export function transformSeries( theme, colorByPrimaryAxis = false, labelPosition, + barMaxWidthPx, } = opts; const contexts = seriesContexts[name || ''] || []; const hasForecast = @@ -603,9 +605,13 @@ export function transformSeries( ...(colorByPrimaryAxis ? {} : { itemStyle }), // @ts-ignore type: plotType, - // 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. - ...(plotType === 'bar' ? { barMaxWidth: 100 } : {}), + // 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 } : {}), smooth: seriesType === 'smooth', triggerLineEvent: true, // @ts-expect-error diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts b/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts index 7454da43b92..5e0ba9d82e3 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts @@ -106,6 +106,12 @@ export const WEEKLY_TIME_GRAINS: ReadonlySet<string> = new Set([ ]); export const TIMEGRAIN_TO_TIMESTAMP = { + [TimeGranularity.SECOND]: 1000, + [TimeGranularity.MINUTE]: 60 * 1000, + [TimeGranularity.FIVE_MINUTES]: 5 * 60 * 1000, + [TimeGranularity.TEN_MINUTES]: 10 * 60 * 1000, + [TimeGranularity.FIFTEEN_MINUTES]: 15 * 60 * 1000, + [TimeGranularity.THIRTY_MINUTES]: 30 * 60 * 1000, [TimeGranularity.HOUR]: 3600 * 1000, [TimeGranularity.DAY]: 3600 * 1000 * 24, [TimeGranularity.MONTH]: 3600 * 1000 * 24 * 31, @@ -113,6 +119,13 @@ export const TIMEGRAIN_TO_TIMESTAMP = { [TimeGranularity.YEAR]: 3600 * 1000 * 24 * 31 * 12, }; +// ECharts' own fallback for a degenerate single-point time-axis domain +// (min === max): it pads by exactly this much on each side, regardless of +// grain (see calcNiceForTimeScale in echarts/lib/scale/Time.js). Bar-width +// sizing for a sparse single-bucket chart mirrors this fixed padding rather +// than guessing at a different visible span. +export const ONE_DAY_MS = 3600 * 1000 * 24; + export const DEFAULT_LEGEND_FORM_DATA: LegendFormData = { legendMargin: null, legendOrientation: LegendOrientation.Top, 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 6ededf12ac3..edd61cf06ac 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts @@ -42,7 +42,9 @@ import type { SeriesOption } from 'echarts'; import { isEmpty, maxBy, meanBy, minBy, orderBy, sumBy } from 'lodash-es'; import { NULL_STRING, + ONE_DAY_MS, StackControlsValue, + TIMEGRAIN_TO_TIMESTAMP, TIMESERIES_CONSTANTS, WEEKLY_TIME_GRAINS, } from '../constants'; @@ -53,6 +55,7 @@ import { StackType, } from '../types'; import { defaultLegendPadding } from '../defaults'; +import { getXAxisDomain } from './formatters'; function isDefined<T>(value: T | undefined | null): boolean { return value !== undefined && value !== null; @@ -1209,6 +1212,56 @@ export function getMinAndMaxFromBounds( return {}; } +/** + * Computes a bar-width cap (px) sized to a temporal x-axis's own resolved + * time-grain bucket, instead of a flat constant that ignores how many + * pixels the grain actually spans on the rendered axis. Returns undefined + * when there isn't enough information to compute a grain-aware width (a + * non-temporal axis, no resolved grain outside TIMEGRAIN_TO_TIMESTAMP, or + * no data), so callers can fall back to their own default in that case. + * + * Uses getXAxisDomain — the same data-extent estimate the x-axis label + * spacing formatter already relies on — for the visible axis span. For two + * or more distinct x-values that's the real ECharts-rendered span (ECharts + * applies no padding there). For a single distinct value (domainMin === + * domainMax), it falls back to 2 * ONE_DAY_MS, mirroring ECharts' own + * degenerate-domain padding for a time axis exactly (calcNiceForTimeScale + * in echarts/lib/scale/Time.js pads a single-point extent by ONE_DAY on + * each side, independent of grain) rather than guessing at a different + * 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. + */ +export function getGrainBarMaxWidth( + xAxisType: AxisType, + resolvedTimeGrain: string | undefined, + dataRecordArrays: Record<string, unknown>[][], + xAxisCol: string, + chartWidth: number, +): number | undefined { + if (xAxisType !== AxisType.Time || !resolvedTimeGrain) { + return undefined; + } + const grainMs = + TIMEGRAIN_TO_TIMESTAMP[ + resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP + ]; + if (!grainMs) { + return undefined; + } + const [domainMin, domainMax] = getXAxisDomain(dataRecordArrays, xAxisCol); + if (domainMin === undefined || domainMax === undefined) { + return undefined; + } + const domainSpanMs = + domainMax > domainMin ? domainMax - domainMin : 2 * ONE_DAY_MS; + const plotWidthPx = Math.max( + chartWidth - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, + 0, + ); + return (grainMs / domainSpanMs) * plotWidthPx; +} + /** * Returns the stackId used in stacked series. * It will return the defaultId if the chart is not using time comparison. 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 84e15fcc71a..c66cb792bec 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 @@ -129,34 +129,36 @@ test('a sparse hourly bucket (two sparse points) does not render several grain-w test('a single sparse hourly bucket (the ticket\'s literal "one hour out of a 24-hour window" case) does not render several grain-widths wider than its own bucket', () => { // getXAxisDomain (data-extent based) is undefined for exactly one point — - // domainMin === domainMax there, so it cannot supply a "visible span" to - // derive a correct pixel width from. The query response's own from_dttm/ - // to_dttm (the actually requested time range, independent of how many - // rows came back) is the one source in the option-object's inputs that - // stays well-defined for a single returned row, so this test uses that as - // the domain instead. Nothing in transformProps.ts currently reads - // from_dttm/to_dttm (confirmed: no reference to either field in - // Timeseries/transformProps.ts, utils/series.ts, or utils/formatters.ts) - // — using it here documents the expectation the fix should meet, not - // behavior the current code already has. + // domainMin === domainMax there, so it can't supply a "visible span" to + // derive a correct pixel width from on its own. Rather than introducing a + // new mechanism for what the visible axis span should be (e.g. reading + // the query response's from_dttm/to_dttm, which transformProps.ts does + // not otherwise consult), this pins the correct width against ECharts' + // own actual, already-existing default for a degenerate single-point time + // axis: verified via a real ECharts SSR/SVG render (`echarts.init(null, + // null, { renderer: 'svg', ssr: true })`, then reading + // `chart.getModel().getComponent('xAxis').axis.scale.getExtent()`) that a + // lone point on a `type: 'time'` axis with no explicit min/max renders + // with a 48-hour (2 * ONE_DAY) extent centered on the point — reproduced + // for PT1H, PT15M, and P1D grains alike (i.e. the padding is fixed and + // grain-independent). This matches reading ECharts' own source directly: + // `calcNiceForTimeScale` in echarts/lib/scale/Time.js pads a degenerate + // extent (`extent[0] === extent[1]`) by exactly `ONE_DAY` on each side. const width = 800; - const fromDttm = Date.UTC(2024, 0, 1, 0, 0, 0); - const toDttm = Date.UTC(2024, 0, 2, 0, 0, 0); // 24h window const singleTimestamp = Date.UTC(2024, 0, 1, 13, 0, 0); - const { series } = buildOptions( - width, - [{ count: 1, __timestamp: singleTimestamp }], - { from_dttm: fromDttm, to_dttm: toDttm }, - ); + const { series } = buildOptions(width, [ + { count: 1, __timestamp: singleTimestamp }, + ]); const [barSeries] = series as BarSeriesOption[]; const plotWidthPx = Math.max( width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0, ); + const verifiedSinglePointDomainSpanMs = 2 * 24 * 60 * 60 * 1000; // 48h const correctGrainPxWidth = - (HOUR_GRAIN_MS / (toDttm - fromDttm)) * plotWidthPx; + (HOUR_GRAIN_MS / verifiedSinglePointDomainSpanMs) * plotWidthPx; expect(effectiveBarPxWidth(barSeries)).toBeLessThanOrEqual( correctGrainPxWidth * 2,
