Skip to content
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

[migration] Fix migration 3dda56f1c #5471

Merged

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Jul 24, 2018

This PR fixes a number of issues with the 3dda56f1c migration which we experienced whilst trying to upgrade our production database. Part of the issues are due to the lack of validation/sanitizing in the form-data and thus erroneous fields may be present.

Specifically this PR:

  • Adds the missing time grains. Note it's unfortunate that we have both week_start_ and week_starting_. Ideally there should only be week_starting_ which is consistent with week_ending_.
  • Fixes the granularity field lookup. Due to erroneous fields we've witnesses form-data with both granularity (Druid) and time_grain_sqla (SQLA) defined (possibly due to switching of datasources). Rather than using the first truthy value it uses the the appropriate field associated with the datasource type.
  • Fixes the period ratio type logic. The period_ratio_type field could be None and thus params.get('period_ratio_type', 'growth') would return None which would result in an exception when trying to call lower(). The fix is simply params.get('period_ratio_type') or 'growth' to ensure that growth is the fallback value.

to: @betodealmeida @graceguo-supercat @mistercrunch @Michelle-Thomas @timifasubaa @williaster

@codecov-io
Copy link

codecov-io commented Jul 24, 2018

Codecov Report

Merging #5471 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5471   +/-   ##
=======================================
  Coverage   59.11%   59.11%           
=======================================
  Files         372      372           
  Lines       23751    23751           
  Branches     2758     2758           
=======================================
  Hits        14041    14041           
  Misses       9695     9695           
  Partials       15       15

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bea0a0a...ae5ad56. Read the comment docs.

@betodealmeida
Copy link
Member

@john-bodley, if I understand correctly you're removing isodate because it's a dependency of another package? But what if we remove that other package in the future? The migration script would still require isodate. What's the benefit of removing it?

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@john-bodley, I think the db_engine_specs_map is wrong.

'week_start_monday': 'P1W',
'week_starting_sunday': 'P1W',
'P1W/1970-01-03T00:00:00Z': 'P1W',
'1969-12-28T00:00:00Z/P1W': 'P1W',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the opposite, since it maps the old format to the new one:

'week_ending_saturday': 'P1W/1970-01-03T00:00:00Z',
'week_start_sunday': '1969-12-28T00:00:00Z/P1W',
'week_start_monday': '1969-12-29T00:00:00Z/P1W',
'week_starting_sunday': '1969-12-28T00:00:00Z/P1W',

@john-bodley
Copy link
Member Author

@betodealmeida the isodate package wasn't added in your PR but rather a follow up PR. I can retract that portion of the change if required.

@john-bodley john-bodley force-pushed the john-bodley-fix-3dda56f1c4c6 branch from a4d2a1b to ae5ad56 Compare July 24, 2018 19:45
@graceguo-supercat graceguo-supercat merged commit bfcc3a6 into apache:master Jul 24, 2018
graceguo-supercat pushed a commit to graceguo-supercat/superset that referenced this pull request Jul 26, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants