codeant-ai-for-open-source[bot] commented on code in PR #39990: URL: https://github.com/apache/superset/pull/39990#discussion_r3213199494
########## superset-frontend/plugins/plugin-chart-echarts/src/Candlestick/controlPanel.ts: ########## @@ -0,0 +1,108 @@ +/** + * 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 { + ControlPanelConfig, + sections, + sharedControls, +} from '@superset-ui/chart-controls'; + +const config: ControlPanelConfig = { + controlPanelSections: [ + { + label: t('Query'), + expanded: true, + controlSetRows: [ + ['temporal_columns_lookup'], + [ + { + name: 'open', + config: { + ...sharedControls.metrics, + label: t('Open'), + description: t('Metric for the opening price'), + multi: false, + clearable: false, + }, + }, + ], + [ + { + name: 'close', + config: { + ...sharedControls.metrics, + label: t('Close'), + description: t('Metric for the closing price'), + multi: false, + clearable: false, + }, + }, + ], + [ + { + name: 'high', + config: { + ...sharedControls.metrics, + label: t('High'), + description: t('Metric for the highest price'), + multi: false, + clearable: false, + }, + }, + ], + [ + { + name: 'low', + config: { + ...sharedControls.metrics, + label: t('Low'), + description: t('Metric for the lowest price'), + multi: false, + clearable: false, + }, + }, + ], + ['adhoc_filters'], + ['row_limit'], Review Comment: **Suggestion:** The query controls omit both `x_axis`/`granularity_sqla` and `time_grain_sqla`, so users cannot configure a time dimension for OHLC rows. This breaks the plugin contract with `buildQuery/getXAxisColumn` and leads to queries without a proper time column (often a single aggregate row or invalid timestamp labels). Add explicit time-axis controls (`x_axis` plus time grain) in the Query section so candlesticks are grouped by time correctly. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Candlestick chart lacks configurable time axis and grain. - ❌ Queries often aggregate OHLC into single undifferentiated candlestick. - ⚠️ Financial time-series analysis unreliable for multi-period datasets. - ⚠️ Inconsistent behavior vs other ECharts time-based plugins. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Register and use the new candlestick plugin, which wires `controlPanel` from `superset-frontend/plugins/plugin-chart-echarts/src/Candlestick/index.ts:21-36` into the Explore UI when a user selects "Candlestick Chart". 2. In Explore, open a dataset with a time column and choose the "Candlestick Chart" viz; observe the Query section rendered from `controlPanel.ts:26-83`, which only exposes `temporal_columns_lookup`, the four OHLC metric pickers (`open`, `close`, `high`, `low`), `adhoc_filters`, and `row_limit`, but no `x_axis`, `granularity_sqla`, or `time_grain_sqla` controls. 3. Run the chart: the query is built by `buildQuery` in `superset-frontend/plugins/plugin-chart-echarts/src/Candlestick/buildQuery.ts:22-39`, which computes `columns: [getXAxisColumn(formData)].filter(Boolean)` and sets `is_timeseries: true`, but since there is no way in this plugin's control panel to specify an x-axis or time grain, `formData` relies on defaults and `getXAxisColumn(formData)` cannot be configured by the user for proper temporal grouping. 4. Because the only user-configurable fields in this plugin are the OHLC metrics and filters, typical usage produces a query that aggregates those metrics at the dataset's default temporal level (often a single time bucket), yielding a single or poorly grouped candlestick series rather than a candle per time interval; adding explicit `x_axis` and `time_grain_sqla` controls in the Query section (matching how `BigNumberWithTrendline/controlPanel.tsx:41-48` exposes `x_axis` and `time_grain_sqla` for its own `getXAxisColumn` usage) would allow users to define a real time dimension and fix this behavior. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fplugins%2Fplugin-chart-echarts%2Fsrc%2FCandlestick%2FcontrolPanel.ts%0A%2A%2ALine%3A%2A%2A%2031%3A82%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20The%20query%20controls%20omit%20both%20%60x_axis%60%2F%60granularity_sqla%60%20and%20%60time_grain_sqla%60%2C%20so%20users%20cannot%20configure%20a%20time%20dimension%20for%20OHLC%20rows.%20This%20breaks%20the%20plugin%20contract%20with%20%60buildQuery%2FgetXAxisColumn%60%20and%20leads%20to%20queries%20without%20a%20proper%20time%20column%20%28often%20a%20single%20aggregate%20row%20or%20invalid%20timestamp%20labels%29.%20Add%20explicit%20time-axis%20controls%20%28%60x_axis%60%20plus%20time%20grain%29%20in%20the%20Query%20section%20so%20candlesticks%20are%20grouped%20by%20time%20correctly.%0A%0AValidate%20the%20correctness%20of%20the%20flagged% 20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fplugins%2Fplugin-chart-echarts%2Fsrc%2FCandlestick%2FcontrolPanel.ts%0A%2A%2ALine%3A%2A%2A%2031%3A82%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20The%20query%20controls%20omit%20both%20%60x_axis%60%2F%60granularity_sqla%60%20and%20%60time_grain_sqla%60%2C%20so%20users%20cannot%20configure%20a%20time %20dimension%20for%20OHLC%20rows.%20This%20breaks%20the%20plugin%20contract%20with%20%60buildQuery%2FgetXAxisColumn%60%20and%20leads%20to%20queries%20without%20a%20proper%20time%20column%20%28often%20a%20single%20aggregate%20row%20or%20invalid%20timestamp%20labels%29.%20Add%20explicit%20time-axis%20controls%20%28%60x_axis%60%20plus%20time%20grain%29%20in%20the%20Query%20section%20so%20candlesticks%20are%20grouped%20by%20time%20correctly.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) *(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/Candlestick/controlPanel.ts **Line:** 31:82 **Comment:** *Incomplete Implementation: The query controls omit both `x_axis`/`granularity_sqla` and `time_grain_sqla`, so users cannot configure a time dimension for OHLC rows. This breaks the plugin contract with `buildQuery/getXAxisColumn` and leads to queries without a proper time column (often a single aggregate row or invalid timestamp labels). Add explicit time-axis controls (`x_axis` plus time grain) in the Query section so candlesticks are grouped by time correctly. 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%2F39990&comment_hash=4f27c74ce935361286d845037e534a13969ca7fdd4db687835e208138dec0d84&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39990&comment_hash=4f27c74ce935361286d845037e534a13969ca7fdd4db687835e208138dec0d84&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]
