-
Notifications
You must be signed in to change notification settings - Fork 16.5k
fix(viz): display offset metrics as dashed lines regardless of compar… #36923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(viz): display offset metrics as dashed lines regardless of compar… #36923
Conversation
…ison_type This fixes a regression where offset metrics (time comparisons like '1 year ago') were not displayed as dashed lines when: - comparison_type was not set to 'Values' - the chart had no dimensions Changes: - Remove comparison_type check in isDerivedSeries.ts - Add exact match handling in timeOffset.ts for dimension-less charts - Update tests to reflect new behavior Fixes apache#36806
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
✅ Deploy Preview for superset-docs-preview canceled.
|
Code Review Agent Run #ff9a91Actionable Suggestions - 0Review 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 |
Nitpicks 🔍
|
superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/isDerivedSeries.ts
Outdated
Show resolved
Hide resolved
superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/isDerivedSeries.ts
Outdated
Show resolved
Hide resolved
| ): boolean => | ||
| typeof series.name === 'string' | ||
| ? !!getTimeOffset(series, timeCompare) | ||
| ? !!getTimeOffset(series, timeCompare) || timeCompare.includes(series.name) | ||
| : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Performance bug: calling getTimeOffset (which scans timeCompare) and then calling timeCompare.includes(...) performs two separate linear scans over timeCompare; compute the offset once and reuse the result to avoid the duplicate O(n) work. [performance]
Severity Level: Minor
| ): boolean => | |
| typeof series.name === 'string' | |
| ? !!getTimeOffset(series, timeCompare) | |
| ? !!getTimeOffset(series, timeCompare) || timeCompare.includes(series.name) | |
| : false; | |
| ): boolean => { | |
| if (typeof series.name !== 'string') return false; | |
| const foundOffset = getTimeOffset(series, timeCompare); | |
| if (foundOffset) return true; | |
| return Array.isArray(timeCompare) && timeCompare.includes(series.name); | |
| }; |
Why it matters? ⭐
Calling getTimeOffset (which iterates timeCompare) and then timeCompare.includes(...) results in two linear scans. Caching the result of getTimeOffset and reusing it avoids the duplicate O(n) work with no behavioral change, so this is a valid micro-optimization.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/timeOffset.ts
**Line:** 37:40
**Comment:**
*Performance: Performance bug: calling `getTimeOffset` (which scans `timeCompare`) and then calling `timeCompare.includes(...)` performs two separate linear scans over `timeCompare`; compute the offset once and reuse the result to avoid the duplicate O(n) work.
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.|
CodeAnt AI finished reviewing your PR. |
…ators/utils/isDerivedSeries.ts Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
…ators/utils/isDerivedSeries.ts Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
Code Review Agent Run #e4959dActionable Suggestions - 0Review 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 |
justinpark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
SUMMARY
Fixes #36806
This PR fixes a regression where offset metrics (time comparisons like "1 year ago") were not displayed as dashed lines when charts have no dimensions.
Root Cause:
The
isDerivedSeriesfunction only applied dashed styling whencomparison_type === "Values", andhasTimeOffsetdidn't detect series where the name exactly matched a time offset (which happens when there are no dimensions).Changes:
comparison_type === "Values"check inisDerivedSeries.tstimeOffset.tsfor dimension-less chartsBEFORE/AFTER
BEFORE

**AFTER **
TESTING INSTRUCTIONS
birth_nameswithds)COUNT(*))2005-01-01 to 2008-12-31)1 year ago1 year ago) appears dashed, not solidADDITIONAL INFORMATION