codeant-ai-for-open-source[bot] commented on code in PR #37794:
URL: https://github.com/apache/superset/pull/37794#discussion_r3501302531
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -105,306 +90,101 @@ export default function transformProps(
startYAxisAtZero,
subheader = '',
subheaderFontSize,
- forceTimestampFormatting,
- yAxisFormat,
- currencyFormat,
- timeRangeFixed,
- showXAxis = false,
- showXAxisMinMaxLabels = false,
- showYAxis = false,
- showYAxisMinMaxLabels = false,
+ alignment = 'center',
} = formData;
- const granularity = extractTimegrain(rawFormData);
- const {
- data = [],
- colnames = [],
- coltypes = [],
- from_dttm: fromDatetime,
- to_dttm: toDatetime,
- detected_currency: detectedCurrency,
- } = queriesData[0];
-
- const aggregatedQueryData = queriesData.length > 1 ? queriesData[1] : null;
-
- const hasAggregatedData =
- aggregatedQueryData?.data &&
- aggregatedQueryData.data.length > 0 &&
- aggregation !== 'LAST_VALUE';
- const aggregatedData = hasAggregatedData ? aggregatedQueryData.data[0] :
null;
+ const { data = [], colnames = [], coltypes = [] } = queriesData[0];
Review Comment:
**Suggestion:** This implementation always consumes `queriesData[0]`, but
the query builder still emits a second query for `aggregation === 'raw'`; that
raw-specific query result is now ignored. This breaks the caller contract
between `buildQuery` and `transformProps` for raw aggregation and can produce
incorrect big-number values when post-processing differs between the two
queries. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Overall value aggregation shows last point, not summary.
⚠️ Distinct-count or ratio KPIs display incorrect big number.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Big Number with Trendline chart plugin wires `buildQuery` and
`transformProps`
together in
`superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/index.ts:21-65`,
so the query objects produced by `buildQuery.ts` are consumed by
`transformProps.ts` for
this chart.
2. In `buildQuery.ts`, when `formData.aggregation === 'raw'`, `buildQuery`
returns two
query objects: the first for the trendline with
pivot/rolling/resample/flatten
post-processing, and a second specifically for raw metrics with
`is_timeseries: false` and
an empty `post_processing` array (see
`superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.ts:35-71`
and the comment at lines 57-59 explaining the raw-specific query structure).
3. The Explore backend executes both queries and passes their results as
`queriesData`
into `transformProps`, but `transformProps.ts` immediately destructures only
`queriesData[0]` (`const { data = [], colnames = [], coltypes = [] } =
queriesData[0];` at
global line 96) and never reads `queriesData[1]`, regardless of the
aggregation setting.
4. When a user selects the “Overall value” aggregation (mapped to
`aggregation === 'raw'`
via the shared `aggregationControl` in
`superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:40-52`),
`computeClientSideAggregation` in `transformProps.ts:44-61` uses
`aggregationChoices.raw.compute` over the time-series values from the first
query, which
simply returns the latest point. The second query’s server-computed “Overall
value” is
ignored, so the bigNumber rendered in BigNumberViz
(`BigNumberViz.tsx:149-157`) shows the
last trendline value rather than the intended overall aggregated value.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=37d189afc14542f5b6239ea69771ffa7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=37d189afc14542f5b6239ea69771ffa7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts
**Line:** 96:96
**Comment:**
*Api Mismatch: This implementation always consumes `queriesData[0]`,
but the query builder still emits a second query for `aggregation === 'raw'`;
that raw-specific query result is now ignored. This breaks the caller contract
between `buildQuery` and `transformProps` for raw aggregation and can produce
incorrect big-number values when post-processing differs between the two
queries.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37794&comment_hash=b5f5523235c663a685acb1af10bbc8093197d7b23f8214d6a4973d6ee90327a7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37794&comment_hash=b5f5523235c663a685acb1af10bbc8093197d7b23f8214d6a4973d6ee90327a7&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -105,306 +90,101 @@ export default function transformProps(
startYAxisAtZero,
subheader = '',
subheaderFontSize,
- forceTimestampFormatting,
- yAxisFormat,
- currencyFormat,
- timeRangeFixed,
- showXAxis = false,
- showXAxisMinMaxLabels = false,
- showYAxis = false,
- showYAxisMinMaxLabels = false,
+ alignment = 'center',
} = formData;
- const granularity = extractTimegrain(rawFormData);
- const {
- data = [],
- colnames = [],
- coltypes = [],
- from_dttm: fromDatetime,
- to_dttm: toDatetime,
- detected_currency: detectedCurrency,
- } = queriesData[0];
-
- const aggregatedQueryData = queriesData.length > 1 ? queriesData[1] : null;
-
- const hasAggregatedData =
- aggregatedQueryData?.data &&
- aggregatedQueryData.data.length > 0 &&
- aggregation !== 'LAST_VALUE';
- const aggregatedData = hasAggregatedData ? aggregatedQueryData.data[0] :
null;
+ const { data = [], colnames = [], coltypes = [] } = queriesData[0];
const refs: Refs = {};
- const metricName = getMetricLabel(metric);
- const metrics = chartProps.datasource?.metrics || [];
- const originalLabel = getOriginalLabel(metric, metrics);
- const showMetricName = chartProps.rawFormData?.show_metric_name ?? false;
- const compareLag = Number(compareLag_) || 0;
- let formattedSubheader = subheader;
- const { r, g, b } = colorPicker;
- const mainColor = `rgb(${r}, ${g}, ${b})`;
+ const metricName = getMetricLabel(metric);
+ const originalLabel = getOriginalLabel(metric, datasource?.metrics || []);
+ const showMetricName = rawFormData?.show_metric_name ?? false;
const xAxisLabel = getXAxisLabel(rawFormData) as string;
- let trendLineData: TimeSeriesDatum[] | undefined;
- let percentChange = 0;
- let bigNumber = data.length === 0 ? null : data[0][metricName];
- let timestamp = data.length === 0 ? null : data[0][xAxisLabel];
- let bigNumberFallback = null;
let sortedData: [number | null, number | null][] = [];
- if (data.length > 0) {
+ if (data.length) {
sortedData = (data as BigNumberDatum[])
.map(
d =>
- [d[xAxisLabel], parseMetricValue(d[metricName])] as [
+ [d[xAxisLabel] as number | null, parseMetricValue(d[metricName])] as
[
number | null,
number | null,
],
)
- // sort in time descending order
.sort((a, b) => (a[0] !== null && b[0] !== null ? b[0] - a[0] : 0));
}
- if (sortedData.length > 0) {
- timestamp = sortedData[0][0];
- // Raw aggregation uses server-side data, all others use client-side
- if (aggregation === 'raw' && hasAggregatedData && aggregatedData) {
- // Use server-side aggregation for raw
- if (
- aggregatedData[metricName] !== null &&
- aggregatedData[metricName] !== undefined
- ) {
- bigNumber = aggregatedData[metricName];
- } else {
- const metricKeys = Object.keys(aggregatedData).filter(
- key =>
- key !== xAxisLabel &&
- aggregatedData[key] !== null &&
- typeof aggregatedData[key] === 'number',
- );
- bigNumber =
- metricKeys.length > 0 ? aggregatedData[metricKeys[0]] : null;
- }
- } else {
- // Use client-side aggregation for all other methods
- bigNumber = computeClientSideAggregation(sortedData, aggregation);
- }
+ let bigNumber = computeClientSideAggregation(sortedData, aggregation);
+ let timestamp = sortedData[0]?.[0] ?? null;
- // Handle null bigNumber case
- if (bigNumber === null) {
- bigNumberFallback = sortedData.find(d => d[1] !== null);
- bigNumber = bigNumberFallback ? bigNumberFallback[1] : null;
- timestamp = bigNumberFallback ? bigNumberFallback[0] : null;
- }
- }
+ let formattedSubheader = subheader;
- if (compareLag > 0 && sortedData.length > 0) {
- const compareIndex = compareLag;
- if (compareIndex < sortedData.length) {
- const compareFromValue = sortedData[compareIndex][1];
- const compareToValue = sortedData[0][1];
- // compare values must both be non-nulls
- if (compareToValue !== null && compareFromValue !== null) {
- percentChange = compareFromValue
- ? (Number(compareToValue) - compareFromValue) /
- Math.abs(compareFromValue)
- : 0;
- formattedSubheader = `${formatPercentChange(
- percentChange,
- )} ${compareSuffix}`;
- }
- }
- }
+ if (compareLag && sortedData.length > compareLag) {
+ const prev = sortedData[compareLag][1];
+ const curr = sortedData[0][1];
- if (data.length > 0 && showTrendLine) {
- // Filter out entries with null timestamps and reverse for chronological
order
- // TimeSeriesDatum requires [number, number | null] - timestamp must be
non-null
- const validData = sortedData.filter(
- (d): d is [number, number | null] => d[0] !== null,
- );
- trendLineData = [...validData].reverse();
+ if (prev != null && curr != null && prev !== 0) {
+ formattedSubheader = `${formatPercentChange(
+ (curr - prev) / Math.abs(prev),
+ )} ${compareSuffix}`;
+ }
}
- let className = '';
- if (percentChange > 0) {
- className = 'positive';
- } else if (percentChange < 0) {
- className = 'negative';
+ let trendLineData: TimeSeriesDatum[] | undefined;
+ if (showTrendLine) {
+ trendLineData = [...sortedData].reverse();
}
- const metricColtypeIndex = colnames.findIndex(name => name === metricName);
- const metricColtype =
- metricColtypeIndex > -1 ? coltypes[metricColtypeIndex] : null;
+ const metricColtype = coltypes[colnames.findIndex(c => c === metricName)];
- let metricEntry: Metric | undefined;
- if (chartProps.datasource?.metrics) {
- metricEntry = chartProps.datasource.metrics.find(
- metricEntry => metricEntry.metric_name === metric,
- );
- }
+ const metricEntry: Metric | undefined = datasource?.metrics?.find(
+ m => m.metric_name === metric,
+ );
const formatTime = getDateFormatter(
timeFormat,
- granularity,
+ extractTimegrain(rawFormData),
metricEntry?.d3format,
);
- if (trendLineData && timeRangeFixed && fromDatetime) {
- const toDatetimeOrToday = toDatetime ?? Date.now();
- if (!trendLineData[0][0] || trendLineData[0][0] > fromDatetime) {
- trendLineData.unshift([fromDatetime, null]);
- }
- if (
- !trendLineData[trendLineData.length - 1][0] ||
- trendLineData[trendLineData.length - 1][0]! < toDatetimeOrToday
- ) {
- trendLineData.push([toDatetimeOrToday, null]);
- }
- }
-
const numberFormatter = getValueFormatter(
metric,
- currencyFormats,
- columnFormats,
- metricEntry?.d3format || yAxisFormat,
- currencyFormat,
- undefined,
- data,
- currencyCodeColumn,
- detectedCurrency,
+ datasource?.currencyFormats,
+ datasource?.columnFormats,
+ metricEntry?.d3format,
);
- const xAxisFormatter = getXAxisFormatter(timeFormat);
- const yAxisFormatter =
- metricColtype === GenericDataType.Temporal ||
- metricColtype === GenericDataType.String ||
- forceTimestampFormatting
- ? formatTime
- : numberFormatter;
- const echartOptions: EChartsCoreOption = trendLineData
- ? {
- series: [
- {
- data: trendLineData,
- type: 'line',
- smooth: true,
- symbol: 'circle',
- symbolSize: 10,
- showSymbol: false,
- color: mainColor,
- areaStyle: {
- color: new graphic.LinearGradient(0, 0, 0, 1, [
- {
- offset: 0,
- color: mainColor,
- },
- {
- offset: 1,
- color: theme.colorBgContainer,
- },
- ]),
- },
- },
- ],
- xAxis: {
- type: 'time',
- show: showXAxis,
- splitLine: {
- show: false,
- },
- axisLabel: {
- hideOverlap: true,
- formatter: xAxisFormatter,
- alignMinLabel: 'left',
- alignMaxLabel: 'right',
- showMinLabel: showXAxisMinMaxLabels,
- showMaxLabel: showXAxisMinMaxLabels,
- },
- },
- yAxis: {
- type: 'value',
- show: showYAxis,
- scale: !startYAxisAtZero,
- splitLine: {
- show: false,
- },
- axisLabel: {
- hideOverlap: true,
- formatter: yAxisFormatter,
- showMinLabel: showYAxisMinMaxLabels,
- showMaxLabel: showYAxisMinMaxLabels,
- },
- },
- grid:
- showXAxis || showYAxis
- ? {
- containLabel: true,
- bottom: TIMESERIES_CONSTANTS.gridOffsetBottom,
- left: TIMESERIES_CONSTANTS.gridOffsetLeft,
- right: TIMESERIES_CONSTANTS.gridOffsetRight,
- top: TIMESERIES_CONSTANTS.gridOffsetTop,
- }
- : {
- bottom: 0,
- left: 0,
- right: 0,
- top: 0,
- },
- tooltip: {
- ...getDefaultTooltip(refs),
- show: !inContextMenu,
- trigger: 'axis',
- formatter: (params: { data: TimeSeriesDatum }[]) =>
- tooltipHtml(
- [
- [
- metricName,
- params[0].data[1] === null
- ? t('N/A')
- : yAxisFormatter.format(params[0].data[1]),
- ],
- ],
- formatTime(params[0].data[0]),
- ),
- },
- aria: {
- enabled: true,
- label: {
- description: `Big number visualization ${subheader}`,
- },
- },
- useUTC: true,
- }
- : {};
+ const headerFormatter =
+ metricColtype === GenericDataType.Temporal ? formatTime : numberFormatter;
- const { onContextMenu } = hooks;
+ const echartOptions: EChartsCoreOption =
+ trendLineData && showTrendLine
+ ? { series: [{ type: 'line', data: trendLineData }] }
+ : {};
Review Comment:
**Suggestion:** The generated ECharts config now only sets a line series and
drops axis/grid configuration, so controls like show X/Y axis, min/max labels,
and y-axis-zero behavior become no-ops at runtime. Restore axis/grid option
construction from formData so the control panel settings actually affect the
rendered chart. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Axis visibility controls have no effect on trendline.
⚠️ Grid padding settings ignored for Big Number trendline.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The form data type for this chart, `BigNumberWithTrendlineFormData`,
includes axis
visibility and min/max label controls (`showXAxis`, `showYAxis`,
`showXAxisMinMaxLabels`,
`showYAxisMinMaxLabels`) in
`superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/types.ts:44-56`,
indicating
these are configurable options users can toggle in the Explore UI.
2. The integration test
`superset-frontend/plugins/plugin-chart-echarts/test/BigNumber/transformProps.test.ts`
exercises these controls: tests at lines 191-233 and 236-252 call
`transformProps` with
different `showXAxis`, `showYAxis`, and min/max label combinations and
assert that
`transformed.echartOptions.xAxis.show`, `yAxis.show`,
`axisLabel.showMinLabel`,
`axisLabel.showMaxLabel`, and `grid` offsets reflect the formData settings.
3. In the current `transformProps.ts`, the formData destructuring at lines
77-93 reads
`showTrendLine` and `startYAxisAtZero` but does not reference any
axis-related flags, and
the constructed `echartOptions` at global lines 161-164 is limited to `{
series: [{ type:
'line', data: trendLineData }] }` or `{}`. No `xAxis`, `yAxis`, or `grid`
properties are
added based on the formData controls.
4. BigNumberViz forwards `echartOptions` unmodified into the shared `Echart`
component
(`superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:213-235`),
and `Echart.tsx` merges these options into the final ECharts configuration
(lines
196-254). Because `echartOptions` lacks axis and grid configuration,
toggling axis
visibility and min/max label controls no longer affects the rendered
trendline, and the
expectations encoded in `test/BigNumber/transformProps.test.ts` cannot be
satisfied.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b75708789a6b4bceb5edaa9bf47c3863&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b75708789a6b4bceb5edaa9bf47c3863&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts
**Line:** 161:164
**Comment:**
*Incomplete Implementation: The generated ECharts config now only sets
a line series and drops axis/grid configuration, so controls like show X/Y
axis, min/max labels, and y-axis-zero behavior become no-ops at runtime.
Restore axis/grid option construction from formData so the control panel
settings actually affect the rendered chart.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37794&comment_hash=f14b6fe6b244d0e0074ef213eb46ea1e8b82ba55b84fd61bba7cbe3f47f09f87&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37794&comment_hash=f14b6fe6b244d0e0074ef213eb46ea1e8b82ba55b84fd61bba7cbe3f47f09f87&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -105,306 +90,101 @@ export default function transformProps(
startYAxisAtZero,
subheader = '',
subheaderFontSize,
- forceTimestampFormatting,
- yAxisFormat,
- currencyFormat,
- timeRangeFixed,
- showXAxis = false,
- showXAxisMinMaxLabels = false,
- showYAxis = false,
- showYAxisMinMaxLabels = false,
+ alignment = 'center',
} = formData;
- const granularity = extractTimegrain(rawFormData);
- const {
- data = [],
- colnames = [],
- coltypes = [],
- from_dttm: fromDatetime,
- to_dttm: toDatetime,
- detected_currency: detectedCurrency,
- } = queriesData[0];
-
- const aggregatedQueryData = queriesData.length > 1 ? queriesData[1] : null;
-
- const hasAggregatedData =
- aggregatedQueryData?.data &&
- aggregatedQueryData.data.length > 0 &&
- aggregation !== 'LAST_VALUE';
- const aggregatedData = hasAggregatedData ? aggregatedQueryData.data[0] :
null;
+ const { data = [], colnames = [], coltypes = [] } = queriesData[0];
const refs: Refs = {};
- const metricName = getMetricLabel(metric);
- const metrics = chartProps.datasource?.metrics || [];
- const originalLabel = getOriginalLabel(metric, metrics);
- const showMetricName = chartProps.rawFormData?.show_metric_name ?? false;
- const compareLag = Number(compareLag_) || 0;
- let formattedSubheader = subheader;
- const { r, g, b } = colorPicker;
- const mainColor = `rgb(${r}, ${g}, ${b})`;
+ const metricName = getMetricLabel(metric);
+ const originalLabel = getOriginalLabel(metric, datasource?.metrics || []);
+ const showMetricName = rawFormData?.show_metric_name ?? false;
const xAxisLabel = getXAxisLabel(rawFormData) as string;
- let trendLineData: TimeSeriesDatum[] | undefined;
- let percentChange = 0;
- let bigNumber = data.length === 0 ? null : data[0][metricName];
- let timestamp = data.length === 0 ? null : data[0][xAxisLabel];
- let bigNumberFallback = null;
let sortedData: [number | null, number | null][] = [];
- if (data.length > 0) {
+ if (data.length) {
sortedData = (data as BigNumberDatum[])
.map(
d =>
- [d[xAxisLabel], parseMetricValue(d[metricName])] as [
+ [d[xAxisLabel] as number | null, parseMetricValue(d[metricName])] as
[
number | null,
number | null,
],
)
- // sort in time descending order
.sort((a, b) => (a[0] !== null && b[0] !== null ? b[0] - a[0] : 0));
}
- if (sortedData.length > 0) {
- timestamp = sortedData[0][0];
- // Raw aggregation uses server-side data, all others use client-side
- if (aggregation === 'raw' && hasAggregatedData && aggregatedData) {
- // Use server-side aggregation for raw
- if (
- aggregatedData[metricName] !== null &&
- aggregatedData[metricName] !== undefined
- ) {
- bigNumber = aggregatedData[metricName];
- } else {
- const metricKeys = Object.keys(aggregatedData).filter(
- key =>
- key !== xAxisLabel &&
- aggregatedData[key] !== null &&
- typeof aggregatedData[key] === 'number',
- );
- bigNumber =
- metricKeys.length > 0 ? aggregatedData[metricKeys[0]] : null;
- }
- } else {
- // Use client-side aggregation for all other methods
- bigNumber = computeClientSideAggregation(sortedData, aggregation);
- }
+ let bigNumber = computeClientSideAggregation(sortedData, aggregation);
+ let timestamp = sortedData[0]?.[0] ?? null;
- // Handle null bigNumber case
- if (bigNumber === null) {
- bigNumberFallback = sortedData.find(d => d[1] !== null);
- bigNumber = bigNumberFallback ? bigNumberFallback[1] : null;
- timestamp = bigNumberFallback ? bigNumberFallback[0] : null;
- }
- }
+ let formattedSubheader = subheader;
- if (compareLag > 0 && sortedData.length > 0) {
- const compareIndex = compareLag;
- if (compareIndex < sortedData.length) {
- const compareFromValue = sortedData[compareIndex][1];
- const compareToValue = sortedData[0][1];
- // compare values must both be non-nulls
- if (compareToValue !== null && compareFromValue !== null) {
- percentChange = compareFromValue
- ? (Number(compareToValue) - compareFromValue) /
- Math.abs(compareFromValue)
- : 0;
- formattedSubheader = `${formatPercentChange(
- percentChange,
- )} ${compareSuffix}`;
- }
- }
- }
+ if (compareLag && sortedData.length > compareLag) {
+ const prev = sortedData[compareLag][1];
+ const curr = sortedData[0][1];
- if (data.length > 0 && showTrendLine) {
- // Filter out entries with null timestamps and reverse for chronological
order
- // TimeSeriesDatum requires [number, number | null] - timestamp must be
non-null
- const validData = sortedData.filter(
- (d): d is [number, number | null] => d[0] !== null,
- );
- trendLineData = [...validData].reverse();
+ if (prev != null && curr != null && prev !== 0) {
+ formattedSubheader = `${formatPercentChange(
+ (curr - prev) / Math.abs(prev),
+ )} ${compareSuffix}`;
+ }
}
- let className = '';
- if (percentChange > 0) {
- className = 'positive';
- } else if (percentChange < 0) {
- className = 'negative';
+ let trendLineData: TimeSeriesDatum[] | undefined;
+ if (showTrendLine) {
+ trendLineData = [...sortedData].reverse();
}
- const metricColtypeIndex = colnames.findIndex(name => name === metricName);
- const metricColtype =
- metricColtypeIndex > -1 ? coltypes[metricColtypeIndex] : null;
+ const metricColtype = coltypes[colnames.findIndex(c => c === metricName)];
- let metricEntry: Metric | undefined;
- if (chartProps.datasource?.metrics) {
- metricEntry = chartProps.datasource.metrics.find(
- metricEntry => metricEntry.metric_name === metric,
- );
- }
+ const metricEntry: Metric | undefined = datasource?.metrics?.find(
+ m => m.metric_name === metric,
+ );
const formatTime = getDateFormatter(
timeFormat,
- granularity,
+ extractTimegrain(rawFormData),
metricEntry?.d3format,
);
- if (trendLineData && timeRangeFixed && fromDatetime) {
- const toDatetimeOrToday = toDatetime ?? Date.now();
- if (!trendLineData[0][0] || trendLineData[0][0] > fromDatetime) {
- trendLineData.unshift([fromDatetime, null]);
- }
- if (
- !trendLineData[trendLineData.length - 1][0] ||
- trendLineData[trendLineData.length - 1][0]! < toDatetimeOrToday
- ) {
- trendLineData.push([toDatetimeOrToday, null]);
- }
- }
-
const numberFormatter = getValueFormatter(
metric,
- currencyFormats,
- columnFormats,
- metricEntry?.d3format || yAxisFormat,
- currencyFormat,
- undefined,
- data,
- currencyCodeColumn,
- detectedCurrency,
+ datasource?.currencyFormats,
+ datasource?.columnFormats,
+ metricEntry?.d3format,
);
- const xAxisFormatter = getXAxisFormatter(timeFormat);
- const yAxisFormatter =
- metricColtype === GenericDataType.Temporal ||
- metricColtype === GenericDataType.String ||
- forceTimestampFormatting
- ? formatTime
- : numberFormatter;
- const echartOptions: EChartsCoreOption = trendLineData
- ? {
- series: [
- {
- data: trendLineData,
- type: 'line',
- smooth: true,
- symbol: 'circle',
- symbolSize: 10,
- showSymbol: false,
- color: mainColor,
- areaStyle: {
- color: new graphic.LinearGradient(0, 0, 0, 1, [
- {
- offset: 0,
- color: mainColor,
- },
- {
- offset: 1,
- color: theme.colorBgContainer,
- },
- ]),
- },
- },
- ],
- xAxis: {
- type: 'time',
- show: showXAxis,
- splitLine: {
- show: false,
- },
- axisLabel: {
- hideOverlap: true,
- formatter: xAxisFormatter,
- alignMinLabel: 'left',
- alignMaxLabel: 'right',
- showMinLabel: showXAxisMinMaxLabels,
- showMaxLabel: showXAxisMinMaxLabels,
- },
- },
- yAxis: {
- type: 'value',
- show: showYAxis,
- scale: !startYAxisAtZero,
- splitLine: {
- show: false,
- },
- axisLabel: {
- hideOverlap: true,
- formatter: yAxisFormatter,
- showMinLabel: showYAxisMinMaxLabels,
- showMaxLabel: showYAxisMinMaxLabels,
- },
- },
- grid:
- showXAxis || showYAxis
- ? {
- containLabel: true,
- bottom: TIMESERIES_CONSTANTS.gridOffsetBottom,
- left: TIMESERIES_CONSTANTS.gridOffsetLeft,
- right: TIMESERIES_CONSTANTS.gridOffsetRight,
- top: TIMESERIES_CONSTANTS.gridOffsetTop,
- }
- : {
- bottom: 0,
- left: 0,
- right: 0,
- top: 0,
- },
- tooltip: {
- ...getDefaultTooltip(refs),
- show: !inContextMenu,
- trigger: 'axis',
- formatter: (params: { data: TimeSeriesDatum }[]) =>
- tooltipHtml(
- [
- [
- metricName,
- params[0].data[1] === null
- ? t('N/A')
- : yAxisFormatter.format(params[0].data[1]),
- ],
- ],
- formatTime(params[0].data[0]),
- ),
- },
- aria: {
- enabled: true,
- label: {
- description: `Big number visualization ${subheader}`,
- },
- },
- useUTC: true,
- }
- : {};
+ const headerFormatter =
+ metricColtype === GenericDataType.Temporal ? formatTime : numberFormatter;
Review Comment:
**Suggestion:** The formatter selection now ignores
`forceTimestampFormatting` (and no longer treats string-typed metric columns as
temporal), so enabling “Force date format” in this chart will not work. Include
the same conditions used in Big Number Total when choosing `headerFormatter` so
date formatting is applied when users explicitly request it. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Big Number Trendline ignores Force date format control.
⚠️ Header date formatting differs from Big Number Total.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Big Number with Trendline chart plugin registers `transformProps` as
its transform
function in
`superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/index.ts:21-65`,
so every Big Number with Trendline visualization uses this code path.
2. The chart form data type `BigNumberWithTrendlineFormData` extends
`BigNumberTotalFormData` and includes `forceTimestampFormatting?: boolean`
(see
`superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/types.ts:37-45`),
meaning
the Explore control panel can set `formData.forceTimestampFormatting` when a
user enables
“Force date format”.
3. In `transformProps.ts`, the function destructures `formData` at lines
77-93 but does
not include `forceTimestampFormatting`, so this user-provided flag never
reaches the
formatter selection logic
(`superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:77-93`).
4. Later in the same file, the metric column type is read from the query
result
(`metricColtype = coltypes[colnames.findIndex(c => c === metricName)]` at
line 50), and
`headerFormatter` is chosen only with `metricColtype ===
GenericDataType.Temporal ?
formatTime : numberFormatter` (lines 69-70 / global 158-159). BigNumberViz
then uses
`headerFormatter` to format the displayed number (see
`superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:149-157`),
so enabling “Force date format” does not change the formatter and
non-temporal or string
metrics remain formatted as plain numbers instead of dates.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=948a1fa86b4042acb8a1fc24cef4ace3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=948a1fa86b4042acb8a1fc24cef4ace3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts
**Line:** 158:159
**Comment:**
*Logic Error: The formatter selection now ignores
`forceTimestampFormatting` (and no longer treats string-typed metric columns as
temporal), so enabling “Force date format” in this chart will not work. Include
the same conditions used in Big Number Total when choosing `headerFormatter` so
date formatting is applied when users explicitly request it.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37794&comment_hash=8ff837a3ff707be5a0bab1b626d8d21b856ae6073f2d937d1e8388765f8344a9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37794&comment_hash=8ff837a3ff707be5a0bab1b626d8d21b856ae6073f2d937d1e8388765f8344a9&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
