[bugfix] fix timegrain addon regression #8165
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8165 +/- ##
=========================================
+ Coverage 66.18% 66.2% +0.02%
=========================================
Files 479 479
Lines 22931 22934 +3
Branches 2524 2524
=========================================
+ Hits 15176 15184 +8
+ Misses 7621 7616 -5
Partials 134 134
Continue to review full report at Codecov.
|
villebro
changed the title
villebro
changed the title
|
This should be ready for review. |
|
Thank you @villebro , it works well (at least for mysql db) |
| @@ -161,7 +138,7 @@ def get_timestamp_expr( | |||
| :return: TimestampExpression object | |||
| """ | |||
| if time_grain: | |||
| time_expr = cls.time_grain_functions.get(time_grain) | |||
| time_expr = cls.get_time_grain_functions().get(time_grain) | |||
There was a problem hiding this comment.
[optional] we may want to materialize the result of get_time_grain_functions() instead of re-computing it every time. This being a static class (all @classmethods) it's a bit funky to do (no constructor). Maybe the @memoize decorator we have around somewhere would work.
May be overkill as this method may not be called much...
There was a problem hiding this comment.
I agree; while this should be minimal overhead, stuff like this tends to cause problems down the line left untreated. To avoid the risk of introducing new regressions I will not refactor this now, but added a TODO that I will try to remove in the near future (will aim to kill this when removing name from TimeGrain).
mistercrunch
added
🍒 0.34.1
🏷️ bot
* Fix regression in time grain addons * Revert privatization of time_grain_functions * Fix test * Rename variable * Fix test * Fix typing error * Refactor and add tests * Add TODO
CATEGORY
SUMMARY
Custom timegrains defined in
superset_config.pystopped working somewhere between 0.32 and 0.34. This fixes the regression, removes an unnecessary private function and renames some functions and variables to more clearly indicate what is public and private. The tests were also lacking, as they only tested private methods, not the actual end result of making changes to the config file. Now both blacklisted and addon grains are tested properly.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Local testing + CI
ADDITIONAL INFORMATION
REVIEWERS
@lilila @mistercrunch