fix: migrate all slices off the old time grain format #9684
Conversation
| duration_dict = name_to_duration(database.grains()) | ||
| granularity = params.get("time_grain_sqla") | ||
| if granularity in duration_dict: | ||
| params["time_grain_sqla"] = duration_dict.get(granularity) |
There was a problem hiding this comment.
Nit. No need to use get() since you checked for the presence of the key on line #101.
| granularity = params.get("time_grain_sqla") | ||
| if granularity in duration_dict: | ||
| params["time_grain_sqla"] = duration_dict.get(granularity) | ||
| slc.params = json.dumps(params, sort_keys=True, indent=4) |
There was a problem hiding this comment.
Let's nix the indent as it unnecessarily bloats the database.
There was a problem hiding this comment.
i added the indent so the params could be more easily editable on the frontend. but we could do the prettifying client side and not put it in the db, so i'll remove it
|
|
||
|
|
||
| @utils.memoized | ||
| def name_to_duration(time_grains): |
There was a problem hiding this comment.
I don't think you want to memoize this signature as different databases may have different mappings for the same time_grains. It seems that,
@utils.memoized
def name_to_duration(database):
time_grains = database.grains()
...
would be safer.
|
|
||
|
|
||
| @utils.memoized | ||
| def duration_to_name(time_grains): |
|
|
||
|
|
||
| @utils.memoized | ||
| def name_to_duration(time_grains): |
There was a problem hiding this comment.
Nit could we call this duration_by_name for consistency when naming dictionary mappings.
etr2460
force-pushed
the
erik-ritter--migrate-time-grain-sqla
branch
from
0269e58
to
120fc86
Compare
Codecov Report
@@ Coverage Diff @@
## master #9684 +/- ##
==========================================
+ Coverage 65.75% 70.57% +4.81%
==========================================
Files 581 581
Lines 30319 30320 +1
Branches 3096 3095 -1
==========================================
+ Hits 19937 21397 +1460
+ Misses 10201 8812 -1389
+ Partials 181 111 -70
Continue to review full report at Codecov.
|
|
|
||
| @utils.memoized | ||
| def duration_by_name(database: Database): | ||
| time_grains = database.grains() |
There was a problem hiding this comment.
Nit. Not required but,
return {grain.name: grain.duration for grain in database.grains()}
is more condensed and easier to grok.
etr2460
force-pushed
the
erik-ritter--migrate-time-grain-sqla
branch
from
120fc86
to
107c5ae
Compare
(cherry picked from commit e95af7f)
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
At Airbnb, we see a decent number of errors like
No grain spec for day for database presto. It appears that this is because we never merged a migration to move slices from the old type to the new one (see #4755 (comment)).I've replicated/updated @jeffreythewang's work (thanks Jeffrey!) from #5485 in this PR to migrate slices to the proper time grain format
TEST PLAN
set
time_grain_sqlafor a chart to "day", run the migration and see it converted to "P1D"ADDITIONAL INFORMATION
REVIEWERS
to: @john-bodley @villebro @mistercrunch @jeffreythewang