codeant-ai-for-open-source[bot] commented on code in PR #40523:
URL: https://github.com/apache/superset/pull/40523#discussion_r3324301723
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts:
##########
@@ -61,6 +61,23 @@ export const DEFAULT_FORM_DATA: EchartsTimeseriesFormData = {
sections.FORECAST_DEFAULT_DATA.forecastSeasonalityWeekly,
forecastSeasonalityYearly:
sections.FORECAST_DEFAULT_DATA.forecastSeasonalityYearly,
+ anomalyDetectionEnabled:
+ sections.ANOMALY_DEFAULT_DATA.anomalyDetectionEnabled,
+ anomalyDetectionMethod: sections.ANOMALY_DEFAULT_DATA.anomalyDetectionMethod,
+ anomalyDetectionRollingWindow:
+ sections.ANOMALY_DEFAULT_DATA.anomalyDetectionRollingWindow,
+ anomalyDetectionSensitivity:
+ sections.ANOMALY_DEFAULT_DATA.anomalyDetectionSensitivity,
+ anomalyDetectionConfidenceInterval:
+ sections.ANOMALY_DEFAULT_DATA.anomalyDetectionConfidenceInterval,
+ anomalyDetectionSeasonalityYearly:
+ sections.ANOMALY_DEFAULT_DATA.anomalyDetectionSeasonalityYearly,
+ anomalyDetectionSeasonalityWeekly:
+ sections.ANOMALY_DEFAULT_DATA.anomalyDetectionSeasonalityWeekly,
+ anomalyDetectionSeasonalityDaily:
+ sections.ANOMALY_DEFAULT_DATA.anomalyDetectionSeasonalityDaily,
+ anomalyDetectionIncludeForecast:
+ sections.ANOMALY_DEFAULT_DATA.anomalyDetectionIncludeForecast,
Review Comment:
**Suggestion:** A new form-data flag for forecast inclusion is initialized
but not wired into query generation, so callers cannot actually control this
behavior. The operator currently forces forecast inclusion based only on
forecast enablement, which makes this new field misleading and causes API
contract mismatch for clients that set it explicitly. Either remove this field
or pass it through and honor it in the anomaly operator options. [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Forecast inclusion flag ignored in anomalyDetectionOperator.ts.
- ⚠️ Callers cannot control anomaly use of forecast data.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note that anomaly detection default form data includes
`anomalyDetectionIncludeForecast`
(superset-frontend/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx:24-34),
and ECharts timeseries form data type declares this field
(superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts:66-75).
2. The timeseries chart plugin's `DEFAULT_FORM_DATA`
(superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts:36-88)
forwards this setting by assigning `anomalyDetectionIncludeForecast:
sections.ANOMALY_DEFAULT_DATA.anomalyDetectionIncludeForecast` (lines
64-80), so consumers
can populate `formData.anomalyDetectionIncludeForecast` when building
queries.
3. Build a chart (or programmatically construct a query) with
`formData.anomalyDetectionEnabled = true`, `formData.forecastEnabled =
true`, and
explicitly set `formData.anomalyDetectionIncludeForecast = false`, then
execute the query
so `buildQuery(formData)` runs and appends
`anomalyDetectionOperator(formData,
baseQueryObject)` to `post_processing` (buildQuery.ts:44-116, especially
102-114).
4. In `anomalyDetectionOperator`
(superset-frontend/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts:25-52),
the options object is built from `anomalyDetectionMethod`, rolling/Prophet
parameters, and
then `include_forecast_data` is set solely based on
`formData.forecastEnabled` (line
46-48). There is no reference to `formData.anomalyDetectionIncludeForecast`
anywhere in
the operator or elsewhere in the codebase (verified via grep), so the
backend always
receives `include_forecast_data = true` whenever forecast is enabled,
regardless of the
new flag, making the option ineffective and the form-data contract
misleading for callers
that set it.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9426578a56a44c5fbb61a92126da7880&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9426578a56a44c5fbb61a92126da7880&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/Timeseries/constants.ts
**Line:** 79:80
**Comment:**
*Incomplete Implementation: A new form-data flag for forecast inclusion
is initialized but not wired into query generation, so callers cannot actually
control this behavior. The operator currently forces forecast inclusion based
only on forecast enablement, which makes this new field misleading and causes
API contract mismatch for clients that set it explicitly. Either remove this
field or pass it through and honor it in the anomaly operator options.
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%2F40523&comment_hash=3302fd1ad0fed5fb72f8730e2fbe801e644fc7e86b564c0080640c6e35122a00&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40523&comment_hash=3302fd1ad0fed5fb72f8730e2fbe801e644fc7e86b564c0080640c6e35122a00&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/buildQuery.ts:
##########
@@ -109,6 +110,7 @@ export default function buildQuery(formData: QueryFormData)
{
flattenOperator(formData, baseQueryObject),
// todo: move prophet before flatten
prophetOperator(formData, baseQueryObject),
+ anomalyDetectionOperator(formData, baseQueryObject),
Review Comment:
**Suggestion:** The new anomaly post-processing step is appended without
guarding for a temporal x-axis when the method is `prophet`. In this flow, a
non-temporal/ad-hoc x-axis can still produce an anomaly rule, and backend
Prophet-based anomaly detection will fail at runtime when it accesses
datetime-only operations on non-datetime data. Add a temporal-axis check (or
block Prophet method unless the x-axis is datetime) before adding this
operator. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Prophet anomaly detection crashes on non-temporal x-axis data.
- ⚠️ Affects ECharts timeseries charts using anomaly_detection.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an ECharts timeseries chart (e.g. Line) whose control panel uses
`sections.echartsTimeSeriesQueryWithXAxisSort`
(superset-frontend/packages/superset-ui-chart-controls/src/sections/echartsTimeSeriesQuery.tsx:47-57),
which exposes an unrestricted `x_axis` control backed by `dndXAxisControl`
(sharedControls.tsx:30, dndControls.tsx:34-37).
2. Set `x_axis` to a non-temporal column or adhoc expression that produces
non-datetime
values; `dndXAxisControl` does not enforce temporal type
(dndControls.tsx:34-37), and
`xAxisMixin` only seeds the initial value (mixins.tsx:6-23).
3. Enable anomaly detection with method `prophet` in form data (controls
defined in
superset-frontend/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx:24-33,
55-63) so that `formData.anomalyDetectionEnabled === true` and
`formData.anomalyDetectionMethod === 'prophet'`, then trigger a query so
`buildQuery()` is
executed (buildQuery.ts:44-116).
4. In `buildQuery()`, the post-processing pipeline always appends
`anomalyDetectionOperator(formData, baseQueryObject)`
(buildQuery.ts:102-114).
`anomalyDetectionOperator`
(superset-frontend/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts:25-52)
calls `getXAxisLabel(formData)` (getXAxis.ts:31-48) and, when
`anomalyDetectionEnabled &&
xAxisLabel`, emits a post-processing step `operation: 'anomaly_detection'`
with
`options.method = 'prophet'` and `options.index = xAxisLabel` pointing to
the non-temporal
column. On the backend, `anomaly_detection()` in
superset/utils/pandas_postprocessing/anomaly.py:194-205 validates only that
`index` exists
( `_validate_anomaly_inputs` at anomaly.py:153-187) but not its dtype, then
`_detect_anomalies_prophet()` (anomaly.py:87-137) executes
`fit_df["ds"].dt.tz`, which
raises an AttributeError on non-datetimelike columns, causing the chart
query to fail at
runtime.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0cfa4294a75d4d35985a4016b37aab02&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0cfa4294a75d4d35985a4016b37aab02&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/Timeseries/buildQuery.ts
**Line:** 113:113
**Comment:**
*Logic Error: The new anomaly post-processing step is appended without
guarding for a temporal x-axis when the method is `prophet`. In this flow, a
non-temporal/ad-hoc x-axis can still produce an anomaly rule, and backend
Prophet-based anomaly detection will fail at runtime when it accesses
datetime-only operations on non-datetime data. Add a temporal-axis check (or
block Prophet method unless the x-axis is datetime) before adding this operator.
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%2F40523&comment_hash=13ebea0425c215cf93f62c7e1e6af5262cb8af94ea1a7840b100cbce9a4e482b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40523&comment_hash=13ebea0425c215cf93f62c7e1e6af5262cb8af94ea1a7840b100cbce9a4e482b&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]
