You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/09/25 01:33:25 UTC
(superset) 05/06: fix(echarts): derive bar width from real grid padding and dataset column metadata
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 <jc...@gmail.com>
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 <no...@anthropic.com>
---
.../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');
});
});