-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
fix: time comparision #19659
fix: time comparision #19659
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19659 +/- ##
==========================================
- Coverage 66.47% 66.37% -0.11%
==========================================
Files 1681 1681
Lines 64468 64468
Branches 6607 6607
==========================================
- Hits 42857 42788 -69
- Misses 19917 19986 +69
Partials 1694 1694
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Tested to work as expected when comparing legacy line chart to v2 line chart. Only one unrelated problem observed when testing with GENERIC_CHART_AXES
set to true on SQLite (the temporal axis is string based, causing the time diff to fail). But this is not a blocker.
@villebro @zhaoyongjie this fixes a fairly major regression (#19116) in my opinion, i.e., we have users reporting incorrect results which triggers a SEV-0 at Airbnb given that the most egregious error for a BI tool is actually reporting misinformation which is far worse than no information and/or an error. I was wondering whether Preset et al. were planning to do a post mortem (or similar) for this? The topic of post mortems might be a good topic to raise during the Superset Operational Model—Quality track. |
@john-bodley, sorry for introducing the regression, I will do a more comprehensive integration test of the query object and advanced analysis. |
@zhaoyongjie out of interest were there no unit tests previously for testing the time comparisons? |
@john-bodley there was unit test for time comparisons, but the query results depended on combination of multiple operators and QueryObject. Unfortunately, there wasn't it. I will add these recently. |
@john-bodley it would be great to get guidelines for what types of regressions require more formal post mortems. If we had these types of guidelines in place, it would be easy to take that into consideration during code review. For instance, anything that affects calculation logic requires sign off from more committers or similar. Also, @zhaoyongjie raised a good point about the previous tests not being specific enough. I think if we strive to make as isolated tests as possible that test for "obvious" cases (in this case that a change is +100, not -100, and that a going from 100 to 200 is +100 % and not, say, +50 % due to using an incorrect denominator in the calculation), the risk for these types of flipped sign regressions would not occur as easily. I'm happy to jump on a call to discuss this or assist in formalizing guidelines for this so we avoid similar problems going forward. |
(cherry picked from commit d7dd411)
SUMMARY
fix time comparison, follow the viz.py pattern
difference: original column - compared column
percentage: (original column - compared column) / compared column
radio: original column / compared column
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION