fix(explore): exclude time-comparison derived columns from stacked Only Total (Closes #43068) - #43071
Conversation
dosubot
Bot
added
explore
Code Review Agent Run #99a70cActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if (Array.from(excludedKeys).some(key => curr !== key && curr.startsWith(key + TIME_COMPARISON_SEPARATOR))) { | ||
| return prev; | ||
| } |
There was a problem hiding this comment.
Suggestion: The prefix check assumes every key beginning with an excluded key and __ is a time-comparison column, but the suffix is never validated against configured time offsets. This can exclude legitimate metrics or columns such as Sort__detail when Sort is a sort-only metric, causing those values to disappear from stacked totals. Restrict the match to known time-comparison-derived keys or pass the configured offsets into this function. [logic error]
Severity Level: Major ⚠️
- ⚠️ Legitimate aliased metrics can disappear from stacked totals.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
**Line:** 413:415
**Comment:**
*Logic Error: The prefix check assumes every key beginning with an excluded key and `__` is a time-comparison column, but the suffix is never validated against configured time offsets. This can exclude legitimate metrics or columns such as `Sort__detail` when `Sort` is a sort-only metric, causing those values to disappear from stacked totals. Restrict the match to known time-comparison-derived keys or pass the configured offsets into this function.
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
|
The flagged issue is correct. The current implementation uses a broad prefix check that incorrectly excludes any column starting with an excluded key followed by the separator, which can lead to the accidental exclusion of legitimate metrics like To resolve this, you should validate that the suffix following the separator is actually a recognized time-comparison offset. If you do not have access to the configured offsets in this function, you should pass them as an argument. Here is a concise implementation of the fix: // Update the exclusion check to validate the suffix
if (Array.from(excludedKeys).some(key =>
curr !== key &&
curr.startsWith(key + TIME_COMPARISON_SEPARATOR) &&
isTimeComparisonOffset(curr.slice((key + TIME_COMPARISON_SEPARATOR).length)) // Add this validation
)) {
return prev;
}I have checked the available review comments, and there are no other comments on this PR. Would you like me to proceed with any other adjustments? superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts |
There was a problem hiding this comment.
Pull request overview
Fixes an Explore stacked “Only Total” inflation bug for time-comparison charts by ensuring extractDataTotalValues excludes time-offset (“derived”) columns belonging to sort-only metrics (e.g. SortMetric__1 year ago) in the ECharts timeseries plugin utilities.
Changes:
- Import
TIME_COMPARISON_SEPARATORto detect time-comparison derived column keys. - Extend
extractDataTotalValuesexclusion logic to skip keys that begin with an excluded metric label plus the time-comparison separator.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Exclude time-comparison derived columns of excluded keys (e.g. SortMetric__1 year ago) | ||
| if (Array.from(excludedKeys).some(key => curr !== key && curr.startsWith(key + TIME_COMPARISON_SEPARATOR))) { | ||
| return prev; |
| import { SupersetTheme } from '@apache-superset/core/theme'; | ||
| import { GenericDataType } from '@apache-superset/core/common'; | ||
| import { SortSeriesType, LegendPaddingType } from '@superset-ui/chart-controls'; | ||
| import { SortSeriesType, LegendPaddingType, TIME_COMPARISON_SEPARATOR } from '@superset-ui/chart-controls'; |
| // Exclude time-comparison derived columns of excluded keys (e.g. SortMetric__1 year ago) | ||
| if (Array.from(excludedKeys).some(key => curr !== key && curr.startsWith(key + TIME_COMPARISON_SEPARATOR))) { | ||
| return prev; |
|
Thanks for taking this on, @waterWang! Couple of things before this is ready. CI's failing on Also, this could use a test covering the |
Summary
#42881 fixed the stacked "Only Total" label so it no longer includes a sort-only metric. It handles the plain and verbose-named cases, but the exclusion is an exact-name match, so time-comparison derived columns of that same sort-only metric are still summed into the total.
Before
extractDataTotalValuescreatesexcludedKeyswith the sort-only metric labels, but a time-comparison derived column likeSortMetric__1 year agodoes not matchSortMetricexactly, so it passes through and inflates the stacked total.After
Check if a column key starts with any excluded key followed by the
TIME_COMPARISON_SEPARATOR(__). This ensures thatSortMetric__1 year ago,SortMetric__2 years ago, etc. are also excluded from the stacked total.Impact
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts—extractDataTotalValuesfunctionCloses #43068