-
Notifications
You must be signed in to change notification settings - Fork 272
refactor: migrate timeseries_limit_metric to legacy_order_by #1364
refactor: migrate timeseries_limit_metric to legacy_order_by #1364
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/B533s6CjW8hxaHryshpXw2CYzndd |
Codecov Report
@@ Coverage Diff @@
## master #1364 +/- ##
==========================================
+ Coverage 30.31% 30.34% +0.03%
==========================================
Files 496 497 +1
Lines 9974 9977 +3
Branches 1678 1679 +1
==========================================
+ Hits 3024 3028 +4
+ Misses 6706 6704 -2
- Partials 244 245 +1
Continue to review full report at Codecov.
|
@@ -53,6 +54,19 @@ export default function normalizeOrderBy(queryObject: QueryObject): QueryObject | |||
}; | |||
} | |||
|
|||
// todo: Removed `legacy_ordery_by` after refactoring |
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.
will be refactored in separate PR .
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 - only comment is should we perform a db migration to avoid breaking existing charts that are leveraging the old sorting control?
ab8ffbb
to
f655aac
Compare
Nevermind! Found it! Thank you! |
🏆 Enhancements
backend changes at: apache/superset#16849
currently, Superset use
timeseries_limit_metric
control for main query orderby. While this is actually prepared for the inner query when viz is timeseries-like.To fix the original incorrect use, so this PR introduce a
legacy_order_by
control.This PR only migrate
pivot table v2
.