-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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: migrate all slices off the old time grain format #9684
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. No need to use get()
since you checked for the presence of the key on line #101.
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.
done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's nix the indent
as it unnecessarily bloats the database.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
good catch, updated
|
||
|
||
@utils.memoized | ||
def duration_to_name(time_grains): |
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.
This is unused.
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.
good catch, thanks!
|
||
|
||
@utils.memoized | ||
def name_to_duration(time_grains): |
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.
Nit could we call this duration_by_name
for consistency when naming dictionary mappings.
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.
done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. Not required but,
return {grain.name: grain.duration for grain in database.grains()}
is more condensed and easier to grok.
120fc86
to
107c5ae
Compare
(cherry picked from commit e95af7f)
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_sqla
for a chart to "day", run the migration and see it converted to "P1D"ADDITIONAL INFORMATION
REVIEWERS
to: @john-bodley @villebro @mistercrunch @jeffreythewang