codeant-ai-for-open-source[bot] commented on code in PR #37643:
URL: https://github.com/apache/superset/pull/37643#discussion_r2760551089
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1183,3 +1183,51 @@ test('should not apply axis bounds calculation when
seriesType is not Bar for ho
// Should not have explicit max set when seriesType is not Bar
expect(xAxisRaw.max).toBeUndefined();
});
+test('should set yAxis min to accommodate negative forecast lower bounds', ()
=> {
+ // Test for Issue #21734: Forecast uncertainty intervals should not be
clipped at zero
+ const queriesData = [
+ {
+ data: [
+ {
+ __timestamp: BASE_TIMESTAMP,
+ metric__yhat: 10,
+ metric__yhat_lower: -5,
+ metric__yhat_upper: 25,
+ },
+ {
+ __timestamp: BASE_TIMESTAMP + 300000000,
+ metric__yhat: 5,
+ metric__yhat_lower: -10,
+ metric__yhat_upper: 20,
+ },
+ {
+ __timestamp: BASE_TIMESTAMP + 600000000,
Review Comment:
**Suggestion:** The test configures `formData.metrics` to include
`"metric"`, but the mocked `queriesData` rows never define a `metric` field,
only `metric__yhat`/`metric__yhat_lower`/`metric__yhat_upper`. This mismatch
means `transformProps` will read `undefined` for the main metric series, which
can lead to empty or invalid series data and unreliable axis min/max
calculations. To align the test data with the metric configuration and ensure
the axis computation logic is actually exercised, add a `metric` value to each
row. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unit test for axis bounds may be flaky or invalid.
- ⚠️ CI test suite reliability reduced for timeseries transforms.
- ⚠️ Forecast negative-interval behavior not properly exercised.
```
</details>
```suggestion
metric: 10,
metric__yhat: 10,
metric__yhat_lower: -5,
metric__yhat_upper: 25,
},
{
__timestamp: BASE_TIMESTAMP + 300000000,
metric: 5,
metric__yhat: 5,
metric__yhat_lower: -10,
metric__yhat_upper: 20,
},
{
__timestamp: BASE_TIMESTAMP + 600000000,
metric: 15,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the test added in transformProps.test.ts which begins at the added
hunk (file:
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts).
The queriesData block containing the rows in question is declared starting
at line 1189
and the `data: [` array begins at line 1190.
2. The test constructs ChartProps with metrics: ['metric'] at lines
1213-1218 (const
chartProps = new ChartProps({... formData: { ...formData, metrics:
['metric'] }, ...})).
This tells transformProps to look for a numeric series field named "metric".
3. The test calls transformProps(...) at lines 1224-1226 (const
transformedProps =
transformProps(chartProps as EchartsTimeseriesChartProps,);). transformProps
will read the
query rows to build series for the metric name provided by formData.
4. However, the mock query rows (lines 1190-1209) only include forecast
columns
metric__yhat, metric__yhat_lower, and metric__yhat_upper — there is no
top-level "metric"
field. Because the test's formData requests a "metric" series,
transformProps will find
undefined/missing main-series values. This can make the axis bounds
calculation rely
solely on other data (or behave inconsistently), and causes the assertion at
lines
1228-1232 (expect(yAxis.min).toBe(-10); expect(yAxis.max).toBeUndefined();)
to be
unreliable or misleading.
5. Fix: add a top-level `metric` numeric value to each mocked row (as in the
improved_code) so transformProps sees a valid main series and the axis
computation is
exercised correctly. If left unmodified, the test may pass or fail depending
on
transformProps handling of missing metric fields, making this a real
test-data mismatch
rather than a production bug.
```
</details>
<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/test/Timeseries/transformProps.test.ts
**Line:** 1193:1204
**Comment:**
*Logic Error: The test configures `formData.metrics` to include
`"metric"`, but the mocked `queriesData` rows never define a `metric` field,
only `metric__yhat`/`metric__yhat_lower`/`metric__yhat_upper`. This mismatch
means `transformProps` will read `undefined` for the main metric series, which
can lead to empty or invalid series data and unreliable axis min/max
calculations. To align the test data with the metric configuration and ensure
the axis computation logic is actually exercised, add a `metric` value to each
row.
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.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -555,6 +573,17 @@ export default function transformProps(
}
}
+ // For all chart types, if we have negative forecast lower bounds,
+ // ensure yAxisMin accommodates them to prevent clipping at zero
+ if (
Review Comment:
**Suggestion:** The new logic that forces `yAxisMin` to `dataMin` when
negative does not guard against `logAxis` being enabled; if users select a log
scale and all series values (including forecast lower bounds) are non-positive
so that `minPositiveValue` is undefined, this block will set a negative `min`
on a logarithmic axis, which ECharts cannot represent and can lead to rendering
errors or an invalid axis configuration. Adding a `!logAxis` guard ensures this
fallback is only applied to linear axes where negative minima are valid. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Timeseries charts with log scale and forecasts fail.
- ❌ Forecast confidence bands may be misrendered or hidden.
- ⚠️ ECharts throws axis configuration errors in console.
- ⚠️ Users must disable log scale to view negative forecasts.
```
</details>
```suggestion
// For all non-log chart types, if we have negative forecast lower bounds,
// ensure yAxisMin accommodates them to prevent clipping at zero
if (
!logAxis &&
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the timeseries transform function at
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts,
and locate the forecast lower-bound min calculation block at lines 367-383
(the code that updates dataMin from ForecastLower series).
2. Create or configure a Timeseries chart with forecasting enabled and a
dataset
where Prophet/prediction produces negative lower confidence bounds
(yhat_lower).
The Timeseries chart runtime calls transformProps(...) which processes
rawSeries
and sets dataMin from forecast lower series at transformProps.ts:367-383.
3. In chart controls enable Log scale (formData.logAxis = true) for the Y
axis.
transformProps keeps the logAxis flag (from formData) and later executes
the
unconditional fallback at transformProps.ts:576-585 which sets yAxisMin =
dataMin.
4. transformProps builds the ECharts yAxis with type set to log when logAxis
is true
(yAxis.type is assigned using logAxis ? AxisType.Log : AxisType.Value in
the same
file),
but yAxis.min is a negative number (set at transformProps.ts:576-585).
ECharts cannot
represent a negative minimum on a logarithmic axis, causing rendering
errors or an
invalid axis configuration (blank chart or console errors) when the chart
is rendered.
```
</details>
<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/Timeseries/transformProps.ts
**Line:** 576:578
**Comment:**
*Logic Error: The new logic that forces `yAxisMin` to `dataMin` when
negative does not guard against `logAxis` being enabled; if users select a log
scale and all series values (including forecast lower bounds) are non-positive
so that `minPositiveValue` is undefined, this block will set a negative `min`
on a logarithmic axis, which ECharts cannot represent and can lead to rendering
errors or an invalid axis configuration. Adding a `!logAxis` guard ensures this
fallback is only applied to linear axes where negative minima are valid.
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.
```
</details>
--
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]
