codeant-ai-for-open-source[bot] commented on code in PR #40523: URL: https://github.com/apache/superset/pull/40523#discussion_r3324294507
########## superset-frontend/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx: ########## @@ -0,0 +1,183 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { t } from '@apache-superset/core/translation'; +import { legacyValidateInteger, legacyValidateNumber } from '@superset-ui/core'; +import { ControlPanelSectionConfig } from '../types'; +import { displayTimeRelatedControls } from '../utils'; + +export const ANOMALY_DEFAULT_DATA = { + anomalyDetectionEnabled: false, + anomalyDetectionMethod: 'zscore', + anomalyDetectionRollingWindow: 14, + anomalyDetectionSensitivity: 3.0, + anomalyDetectionConfidenceInterval: 0.8, + anomalyDetectionSeasonalityYearly: null, + anomalyDetectionSeasonalityWeekly: null, + anomalyDetectionSeasonalityDaily: null, + anomalyDetectionIncludeForecast: false, +}; + +export const anomalyDetectionControls: ControlPanelSectionConfig = { + label: t('Anomaly Detection'), + expanded: false, + visibility: displayTimeRelatedControls, + controlSetRows: [ + [ + { + name: 'anomalyDetectionEnabled', + config: { + type: 'CheckboxControl', + label: t('Enable anomaly detection'), + renderTrigger: false, + default: ANOMALY_DEFAULT_DATA.anomalyDetectionEnabled, + description: t('Enable anomaly detection on the time series'), + }, + }, + ], + [ + { + name: 'anomalyDetectionMethod', + config: { + type: 'SelectControl', + label: t('Detection method'), + choices: [ + ['zscore', t('Z-Score')], + ['mad', t('MAD (Median Absolute Deviation)')], + ['prophet', t('Prophet (Seasonality-aware)')], + ], + default: ANOMALY_DEFAULT_DATA.anomalyDetectionMethod, + description: t( + 'Algorithm to use for anomaly detection. Z-Score uses rolling mean and standard deviation. MAD uses rolling median absolute deviation which is more robust to outliers. Prophet uses Facebook Prophet to model seasonality and flags points outside the confidence interval.', + ), + }, + }, + ], + [ + { + name: 'anomalyDetectionRollingWindow', + config: { + type: 'TextControl', + label: t('Rolling window'), + validators: [legacyValidateInteger], Review Comment: **Suggestion:** The UI only validates integer format for rolling window but not the required minimum (`>= 3`), so users can submit values that pass frontend validation but always fail in backend post-processing. Add a minimum-bound validator to align frontend and backend contracts. [incorrect condition logic] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Rolling-window anomalies with window<3 always error server-side. - ⚠️ Users receive backend errors instead of inline form feedback. - ⚠️ Misaligned validation complicates troubleshooting for chart authors. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open any ECharts Timeseries chart with anomaly detection enabled in its control panel configuration (e.g. Regular Line at `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx:61-68` includes `sections.anomalyDetectionControls`). 2. In the "Anomaly Detection" section (`superset-frontend/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx:36-85`), enable `anomalyDetectionEnabled` and keep the default method `'zscore'` or `'mad'`. In the "Rolling window" `TextControl` configuration (lines 73-81) the only validator is `legacyValidateInteger` (line 77) which, per its implementation (`superset-frontend/packages/superset-ui-core/src/validator/legacyValidateInteger.ts:26-33`), ensures only that the value is an integer when present but does not enforce any minimum bound. Enter `2` as the rolling window value; this passes UI validation because `parseInt('2', 10)` equals `Number('2')`. 3. When running the query, `buildQuery` (`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/buildQuery.ts:44-114`) calls `anomalyDetectionOperator(formData, baseQueryObject)`. In `anomalyDetectionOperator`'s non-Prophet branch (`superset-frontend/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts:39-45`), the rolling window is parsed as `options.rolling_window = parseInt(formData.anomalyDetectionRollingWindow, 10);` (lines 40-43), resulting in `rolling_window=2` in the `post_processing` options sent to the backend. 4. The backend applies `anomaly_detection` (`superset/utils/pandas_postprocessing/anomaly.py:194-205`). Inside `_validate_anomaly_inputs` (`superset/utils/pandas_postprocessing/anomaly.py:170-174`), for methods `"zscore"` and `"mad"`, the check `if not isinstance(rolling_window, int) or rolling_window < 3:` is enforced. With `rolling_window=2`, this triggers `InvalidPostProcessingError("Rolling window must be an integer >= 3")`, exactly as asserted in `test_anomaly_detection_rolling_window_too_small` (`tests/unit_tests/pandas_postprocessing/test_anomaly.py:128-135`). Thus the frontend happily accepts a value (`2`) that is guaranteed to cause a server-side error on query execution. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4a6fc967d80845eeb0606b89e2dc4fa0&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=4a6fc967d80845eeb0606b89e2dc4fa0&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/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx **Line:** 77:77 **Comment:** *Incorrect Condition Logic: The UI only validates integer format for rolling window but not the required minimum (`>= 3`), so users can submit values that pass frontend validation but always fail in backend post-processing. Add a minimum-bound validator to align frontend and backend contracts. 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=bed59320cf0b7d23b836f84db98aceaa410ba3a4825bbe3ec614bb8c2c8e9e6d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40523&comment_hash=bed59320cf0b7d23b836f84db98aceaa410ba3a4825bbe3ec614bb8c2c8e9e6d&reaction=dislike'>👎</a> ########## superset-frontend/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts: ########## @@ -0,0 +1,55 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + PostProcessingAnomalyDetection, + getXAxisLabel, +} from '@superset-ui/core'; +import { PostProcessingFactory } from './types'; + +export const anomalyDetectionOperator: PostProcessingFactory< + PostProcessingAnomalyDetection +> = (formData, queryObject) => { + const xAxisLabel = getXAxisLabel(formData); + if (formData.anomalyDetectionEnabled && xAxisLabel) { + const method: string = formData.anomalyDetectionMethod || 'zscore'; + const options: Record<string, unknown> = { method, index: xAxisLabel }; + if (method === 'prophet') { + options.confidence_interval = parseFloat( + formData.anomalyDetectionConfidenceInterval ?? 0.8, + ); Review Comment: **Suggestion:** Using `?? 0.8` does not protect against empty string input, so `parseFloat('')` produces `NaN` and sends `null` `confidence_interval`, which fails backend validation for Prophet anomaly detection. Parse then validate as finite, or coerce empty-string values to the default before parsing. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Prophet-based anomaly queries fail when CI cleared. - ⚠️ Users see server error despite valid-looking UI state. - ⚠️ Dashboard charts using Prophet anomalies can break. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open an ECharts Timeseries chart whose control panel includes anomaly detection controls (e.g. Regular Line at `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx:61-68` with `sections.anomalyDetectionControls`). 2. In `Anomaly Detection` (`superset-frontend/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx:36-116`), enable `anomalyDetectionEnabled` and choose `'prophet'` in the "Detection method" select (lines 55-63). This reveals the "Confidence interval" `TextControl` configured with `validators: [legacyValidateNumber]` and default `0.8` (lines 105-111). Clear this field so its value becomes an empty string. Because `legacyValidateNumber` only flags non-empty non-numeric strings (`superset-frontend/packages/superset-ui-core/src/validator/legacyValidateNumber.ts:26-30` and its test at `test/validator/legacyValidateNumber.test.ts:28-35` shows `''` returns `false`), the form allows submission with an empty confidence interval. 3. On query execution, `buildQuery` (`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/buildQuery.ts:44-114`) calls `anomalyDetectionOperator(formData, baseQueryObject)`. In the Prophet branch of `anomalyDetectionOperator` (`superset-frontend/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts:32-38`), the confidence interval is computed as: - `options.confidence_interval = parseFloat(formData.anomalyDetectionConfidenceInterval ?? 0.8);` (lines 33-35). Because the field is the empty string `''`, the nullish coalescing `?? 0.8` uses `''` instead of the default, and `parseFloat('')` returns `NaN`. This `NaN` is stored in `options.confidence_interval` in the `post_processing` rule. 4. When serialized to JSON, `NaN` becomes `null`, so the backend receives `confidence_interval=None` in the `anomaly_detection` call (`superset/utils/pandas_postprocessing/anomaly.py:194-205`). In `_validate_anomaly_inputs` for method `"prophet"` (`superset/utils/pandas_postprocessing/anomaly.py:177-185`), the condition `if (not confidence_interval or confidence_interval <= 0 or confidence_interval >= 1)` treats `None` as falsey (`not confidence_interval`), raising `InvalidPostProcessingError("Confidence interval must be between 0 and 1 (exclusive)")`. This exact behavior is covered by `test_anomaly_detection_prophet_invalid_confidence` (`tests/unit_tests/pandas_postprocessing/test_anomaly.py:240-259`), confirming that a null/invalid confidence interval causes a backend failure instead of letting the default `0.8` apply. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1c4e11493de74b3aa17dd0f187920424&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=1c4e11493de74b3aa17dd0f187920424&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/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts **Line:** 33:35 **Comment:** *Type Error: Using `?? 0.8` does not protect against empty string input, so `parseFloat('')` produces `NaN` and sends `null` `confidence_interval`, which fails backend validation for Prophet anomaly detection. Parse then validate as finite, or coerce empty-string values to the default before parsing. 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=5f3ef3960c03405f62667cfd5d60d0b89eaacf8e995441a31ba871d1101f3d78&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40523&comment_hash=5f3ef3960c03405f62667cfd5d60d0b89eaacf8e995441a31ba871d1101f3d78&reaction=dislike'>👎</a> ########## superset-frontend/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx: ########## @@ -0,0 +1,183 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { t } from '@apache-superset/core/translation'; +import { legacyValidateInteger, legacyValidateNumber } from '@superset-ui/core'; +import { ControlPanelSectionConfig } from '../types'; +import { displayTimeRelatedControls } from '../utils'; + +export const ANOMALY_DEFAULT_DATA = { + anomalyDetectionEnabled: false, + anomalyDetectionMethod: 'zscore', + anomalyDetectionRollingWindow: 14, + anomalyDetectionSensitivity: 3.0, + anomalyDetectionConfidenceInterval: 0.8, + anomalyDetectionSeasonalityYearly: null, + anomalyDetectionSeasonalityWeekly: null, + anomalyDetectionSeasonalityDaily: null, + anomalyDetectionIncludeForecast: false, +}; + +export const anomalyDetectionControls: ControlPanelSectionConfig = { + label: t('Anomaly Detection'), + expanded: false, + visibility: displayTimeRelatedControls, + controlSetRows: [ + [ + { + name: 'anomalyDetectionEnabled', + config: { + type: 'CheckboxControl', + label: t('Enable anomaly detection'), + renderTrigger: false, + default: ANOMALY_DEFAULT_DATA.anomalyDetectionEnabled, + description: t('Enable anomaly detection on the time series'), + }, + }, + ], + [ + { + name: 'anomalyDetectionMethod', + config: { + type: 'SelectControl', + label: t('Detection method'), + choices: [ + ['zscore', t('Z-Score')], + ['mad', t('MAD (Median Absolute Deviation)')], + ['prophet', t('Prophet (Seasonality-aware)')], + ], + default: ANOMALY_DEFAULT_DATA.anomalyDetectionMethod, + description: t( + 'Algorithm to use for anomaly detection. Z-Score uses rolling mean and standard deviation. MAD uses rolling median absolute deviation which is more robust to outliers. Prophet uses Facebook Prophet to model seasonality and flags points outside the confidence interval.', + ), + }, + }, + ], + [ + { + name: 'anomalyDetectionRollingWindow', + config: { + type: 'TextControl', + label: t('Rolling window'), + validators: [legacyValidateInteger], + default: ANOMALY_DEFAULT_DATA.anomalyDetectionRollingWindow, + description: t( + 'Size of the rolling window for computing statistics. Must be >= 3.', + ), + visibility: ({ controls }) => + controls?.anomalyDetectionMethod?.value !== 'prophet', + }, + }, + ], + [ + { + name: 'anomalyDetectionSensitivity', + config: { + type: 'TextControl', + label: t('Sensitivity'), + validators: [legacyValidateNumber], + default: ANOMALY_DEFAULT_DATA.anomalyDetectionSensitivity, + description: t( + 'Threshold for anomaly detection. Higher values mean fewer anomalies are detected. Typical values: 2.0 (more sensitive) to 4.0 (less sensitive).', + ), + visibility: ({ controls }) => + controls?.anomalyDetectionMethod?.value !== 'prophet', + }, + }, + ], + [ + { + name: 'anomalyDetectionConfidenceInterval', + config: { + type: 'TextControl', + label: t('Confidence interval'), + validators: [legacyValidateNumber], Review Comment: **Suggestion:** Confidence interval is validated only as numeric, not as `(0, 1)`, so out-of-range values can be submitted and then hard-fail in backend Prophet validation. Enforce the range in the frontend validator to prevent runtime query errors. [incorrect condition logic] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Prophet anomaly queries fail for CI 0,1,>1 from UI. - ⚠️ Users receive cryptic backend errors for out-of-range values. - ⚠️ Frontend-backend contract mismatch degrades anomaly UX reliability. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open an ECharts Timeseries chart that uses anomaly detection controls (for instance the Area chart, which includes `sections.anomalyDetectionControls` in its `controlPanelSections` at `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx:62-69`). 2. In the "Anomaly Detection" section (`superset-frontend/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx:103-116`), select the `'prophet'` detection method so the "Confidence interval" `TextControl` becomes visible. This control is configured with `validators: [legacyValidateNumber]` and a descriptive string "Should be between 0 and 1" (lines 105-113) but no actual range validator, and `legacyValidateNumber` (`superset-frontend/packages/superset-ui-core/src/validator/legacyValidateNumber.ts:26-30`) only checks that the input is numeric. Enter values like `0`, `1.0`, or `1.5` in the Confidence interval field; these are numeric and therefore pass the current validator with no UI error. 3. When executing the query, `buildQuery` (`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/buildQuery.ts:44-114`) again calls `anomalyDetectionOperator(formData, baseQueryObject)`. In the Prophet-specific branch (`superset-frontend/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts:32-38`), the confidence interval is parsed as `options.confidence_interval = parseFloat(formData.anomalyDetectionConfidenceInterval ?? 0.8);` (lines 33-35), so the selected values (`0`, `1.0`, `1.5`) are passed through unchanged into the `post_processing` rule. 4. The backend `anomaly_detection` function (`superset/utils/pandas_postprocessing/anomaly.py:194-205`) passes these values into `_validate_anomaly_inputs` for the `"prophet"` method (lines 177-185). That function requires `confidence_interval` to be strictly between `0` and `1`: `if (not confidence_interval or confidence_interval <= 0 or confidence_interval >= 1): raise InvalidPostProcessingError("Confidence interval must be between 0 and 1 (exclusive)")`. The unit test `test_anomaly_detection_prophet_invalid_confidence` (`tests/unit_tests/pandas_postprocessing/test_anomaly.py:240-259`) specifically asserts that values `0`, `1.0`, and `1.5` all raise `InvalidPostProcessingError`. Thus, the current UI allows out-of-range confidence intervals that always break Prophet-based anomaly detection at runtime. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=343197bb1b4a4e35875eb4ed0ce1f747&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=343197bb1b4a4e35875eb4ed0ce1f747&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/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx **Line:** 109:109 **Comment:** *Incorrect Condition Logic: Confidence interval is validated only as numeric, not as `(0, 1)`, so out-of-range values can be submitted and then hard-fail in backend Prophet validation. Enforce the range in the frontend validator to prevent runtime query errors. 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=d360609b07f98da6ae7f4206f653d6c35e612bcd08b08d272fc4cf748f4e79da&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40523&comment_hash=d360609b07f98da6ae7f4206f653d6c35e612bcd08b08d272fc4cf748f4e79da&reaction=dislike'>👎</a> ########## superset-frontend/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts: ########## @@ -0,0 +1,55 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + PostProcessingAnomalyDetection, + getXAxisLabel, +} from '@superset-ui/core'; +import { PostProcessingFactory } from './types'; + +export const anomalyDetectionOperator: PostProcessingFactory< + PostProcessingAnomalyDetection +> = (formData, queryObject) => { + const xAxisLabel = getXAxisLabel(formData); + if (formData.anomalyDetectionEnabled && xAxisLabel) { + const method: string = formData.anomalyDetectionMethod || 'zscore'; + const options: Record<string, unknown> = { method, index: xAxisLabel }; + if (method === 'prophet') { + options.confidence_interval = parseFloat( + formData.anomalyDetectionConfidenceInterval ?? 0.8, + ); + options.yearly_seasonality = formData.anomalyDetectionSeasonalityYearly; + options.weekly_seasonality = formData.anomalyDetectionSeasonalityWeekly; + options.daily_seasonality = formData.anomalyDetectionSeasonalityDaily; + } else { + options.rolling_window = parseInt( + formData.anomalyDetectionRollingWindow, + 10, + ); + options.sensitivity = parseFloat(formData.anomalyDetectionSensitivity); Review Comment: **Suggestion:** `rolling_window` and `sensitivity` are parsed without fallback handling, so missing/empty control values become `NaN` in JS (serialized as `null`) and override backend defaults, which then triggers `InvalidPostProcessingError` for zscore/mad requests. Default these values before parsing (or omit the keys when invalid) so backend defaults can safely apply. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Z-Score/MAD anomaly queries fail when fields emptied. - ⚠️ Explore view shows backend error instead of rendering chart. - ⚠️ Dashboards using such configs can intermittently error. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open any ECharts Timeseries chart (for example Regular Line) whose control panel includes `sections.anomalyDetectionControls` (see `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx:61-68` where `sections.anomalyDetectionControls` is added). 2. In the "Anomaly Detection" section (`superset-frontend/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx:36-83`), enable `anomalyDetectionEnabled` and select a non-Prophet method such as `'zscore'` or `'mad'`. Then clear the "Rolling window" and "Sensitivity" text fields so they become empty strings. These fields use `validators: [legacyValidateInteger]` and `validators: [legacyValidateNumber]` respectively (lines 73-80 and 89-96) which treat `''` as valid per `legacyValidateInteger`/`legacyValidateNumber` implementations (`superset-frontend/packages/superset-ui-core/src/validator/legacyValidateInteger.ts:26-33` and `legacyValidateNumber.ts:26-30`), so the form submits without client-side error. 3. When the chart query is run, the Timeseries `buildQuery` function (`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/buildQuery.ts:44-114`) calls `anomalyDetectionOperator(formData, baseQueryObject)` (import on line 35). Inside `anomalyDetectionOperator` (`superset-frontend/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts:25-45`), the non-Prophet branch executes: - `options.rolling_window = parseInt(formData.anomalyDetectionRollingWindow, 10);` (lines 40-43) → `parseInt('' ,10)` returns `NaN`. - `options.sensitivity = parseFloat(formData.anomalyDetectionSensitivity);` (line 44) → `parseFloat('')` returns `NaN`. These `NaN` values are placed into the `options` object that becomes part of the `post_processing` array in the query payload. 4. When the query is JSON-serialized and sent to the backend, the `NaN` values in `options.rolling_window` and `options.sensitivity` are converted to `null` in JSON. The backend post-processing layer then calls `anomaly_detection` from `superset.utils.pandas_postprocessing` (`superset/utils/pandas_postprocessing/__init__.py:17-18` and `anomaly.py:194-205`) with `rolling_window=None` and `sensitivity=None`. In `_validate_anomaly_inputs` (`superset/utils/pandas_postprocessing/anomaly.py:153-176`), for methods `"zscore"` and `"mad"`, `rolling_window` must be an `int >= 3` and `sensitivity` must be a positive number. With `None` values coming from the `null` JSON, these checks fail (`not isinstance(rolling_window, int)` and `not isinstance(sensitivity, (int, float))`), raising `InvalidPostProcessingError` exactly as exercised in the unit tests `test_anomaly_detection_rolling_window_too_small` and `test_anomaly_detection_invalid_sensitivity` (`tests/unit_tests/pandas_postprocessing/test_anomaly.py:128-135` and `138-153`). The result is a runtime query failure for anomaly-enabled zscore/mad charts whenever the user clears these fields. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=092f36003a1541f6844db7df4e4d898a&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=092f36003a1541f6844db7df4e4d898a&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/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts **Line:** 40:44 **Comment:** *Type Error: `rolling_window` and `sensitivity` are parsed without fallback handling, so missing/empty control values become `NaN` in JS (serialized as `null`) and override backend defaults, which then triggers `InvalidPostProcessingError` for zscore/mad requests. Default these values before parsing (or omit the keys when invalid) so backend defaults can safely apply. 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=c174ea686f7deb423e117885cffe43b684f1235c2d08756ec52133a1a339a41f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40523&comment_hash=c174ea686f7deb423e117885cffe43b684f1235c2d08756ec52133a1a339a41f&reaction=dislike'>👎</a> ########## superset-frontend/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx: ########## @@ -0,0 +1,183 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { t } from '@apache-superset/core/translation'; +import { legacyValidateInteger, legacyValidateNumber } from '@superset-ui/core'; +import { ControlPanelSectionConfig } from '../types'; +import { displayTimeRelatedControls } from '../utils'; + +export const ANOMALY_DEFAULT_DATA = { + anomalyDetectionEnabled: false, + anomalyDetectionMethod: 'zscore', + anomalyDetectionRollingWindow: 14, + anomalyDetectionSensitivity: 3.0, + anomalyDetectionConfidenceInterval: 0.8, + anomalyDetectionSeasonalityYearly: null, + anomalyDetectionSeasonalityWeekly: null, + anomalyDetectionSeasonalityDaily: null, + anomalyDetectionIncludeForecast: false, +}; + +export const anomalyDetectionControls: ControlPanelSectionConfig = { + label: t('Anomaly Detection'), + expanded: false, + visibility: displayTimeRelatedControls, + controlSetRows: [ + [ + { + name: 'anomalyDetectionEnabled', + config: { + type: 'CheckboxControl', + label: t('Enable anomaly detection'), + renderTrigger: false, + default: ANOMALY_DEFAULT_DATA.anomalyDetectionEnabled, + description: t('Enable anomaly detection on the time series'), + }, + }, + ], + [ + { + name: 'anomalyDetectionMethod', + config: { + type: 'SelectControl', + label: t('Detection method'), + choices: [ + ['zscore', t('Z-Score')], + ['mad', t('MAD (Median Absolute Deviation)')], + ['prophet', t('Prophet (Seasonality-aware)')], + ], + default: ANOMALY_DEFAULT_DATA.anomalyDetectionMethod, + description: t( + 'Algorithm to use for anomaly detection. Z-Score uses rolling mean and standard deviation. MAD uses rolling median absolute deviation which is more robust to outliers. Prophet uses Facebook Prophet to model seasonality and flags points outside the confidence interval.', + ), + }, + }, + ], + [ + { + name: 'anomalyDetectionRollingWindow', + config: { + type: 'TextControl', + label: t('Rolling window'), + validators: [legacyValidateInteger], + default: ANOMALY_DEFAULT_DATA.anomalyDetectionRollingWindow, + description: t( + 'Size of the rolling window for computing statistics. Must be >= 3.', + ), + visibility: ({ controls }) => + controls?.anomalyDetectionMethod?.value !== 'prophet', + }, + }, + ], + [ + { + name: 'anomalyDetectionSensitivity', + config: { + type: 'TextControl', + label: t('Sensitivity'), + validators: [legacyValidateNumber], Review Comment: **Suggestion:** Sensitivity is only validated as numeric, but backend requires it to be strictly positive; zero/negative values are currently accepted by the control and then rejected during query execution. Add a positive-number validator in the control config. [incorrect condition logic] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Z-Score/MAD anomalies with sensitivity<=0 always fail backend. - ⚠️ Users can configure invalid sensitivities without UI warnings. - ⚠️ Chart authors must debug server errors for simple misinputs. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open an ECharts Timeseries chart that includes anomaly detection controls (e.g. Regular Line, which uses `sections.anomalyDetectionControls` as shown in `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx:61-68`). 2. In the "Anomaly Detection" controls (`superset-frontend/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx:36-102`), enable `anomalyDetectionEnabled` with method `'zscore'` or `'mad'`. The "Sensitivity" `TextControl` configuration (lines 89-99) uses only `validators: [legacyValidateNumber]` (line 93), whose implementation (`superset-frontend/packages/superset-ui-core/src/validator/legacyValidateNumber.ts:26-30`) merely checks that non-empty values are numeric. Enter `0` or `-1` in the Sensitivity field: both are numeric, so `legacyValidateNumber` returns `false` (no error), and the form accepts these non-positive values. 3. When the query runs, `buildQuery` (`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/buildQuery.ts:44-114`) calls `anomalyDetectionOperator(formData, baseQueryObject)`. In the non-Prophet branch (`superset-frontend/packages/superset-ui-chart-controls/src/operators/anomalyDetectionOperator.ts:39-45`), sensitivity is parsed as `options.sensitivity = parseFloat(formData.anomalyDetectionSensitivity);` (line 44), resulting in `sensitivity=0` or `sensitivity=-1` in the `post_processing` rule sent to the backend. 4. The backend `anomaly_detection` function (`superset/utils/pandas_postprocessing/anomaly.py:194-205`) invokes `_validate_anomaly_inputs` (lines 153-176), which for `"zscore"`/`"mad"` requires `sensitivity` to be a positive number: `if not isinstance(sensitivity, (int, float)) or sensitivity <= 0: raise InvalidPostProcessingError("Sensitivity must be a positive number")`. The unit test `test_anomaly_detection_invalid_sensitivity` (`tests/unit_tests/pandas_postprocessing/test_anomaly.py:138-153`) explicitly confirms that sensitivity `-1.0` and `0` both raise `InvalidPostProcessingError`. Therefore, any user-entered non-positive sensitivity that passes the current frontend validator is guaranteed to fail anomaly queries at runtime. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fdc0d4a12a54498584f6db87490ff6c6&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=fdc0d4a12a54498584f6db87490ff6c6&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/packages/superset-ui-chart-controls/src/sections/anomalyDetection.tsx **Line:** 93:93 **Comment:** *Incorrect Condition Logic: Sensitivity is only validated as numeric, but backend requires it to be strictly positive; zero/negative values are currently accepted by the control and then rejected during query execution. Add a positive-number validator in the control config. 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=b7e806adc5e9954398b62febdf31b914f98d531b995aaee0eba7e43d21424efa&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40523&comment_hash=b7e806adc5e9954398b62febdf31b914f98d531b995aaee0eba7e43d21424efa&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]
