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.

Sign up for GitHub

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

Jump to bottom

fix: Fix migration for removing time_range_endpoints 3 #19767

Merged
merged 5 commits into from
Apr 19, 2022
Merged

fix: Fix migration for removing time_range_endpoints 3 #19767

merged 5 commits into from
Apr 19, 2022

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Apr 18, 2022 •
edited
Loading

SUMMARY

Update on #19728 to also remove time_range_endpoints from form_data payload as well.

Benchmarking Results:

Results:

Current: 0.44 s
10+: 0.37 s
100+: 0.35 s
1000+: 0.34 s

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@hughhhh hughhhh requested a review from a team as a code owner April 18, 2022 18:38
@eschutho
Copy link
Member

do you want to make cecc6bf46990 a noop again?

@codecov
Copy link

codecov bot commented Apr 18, 2022 •
edited
Loading

Codecov Report

Merging #19767 (5ed0c10) into master (cf51459) will increase coverage by 0.00%.
The diff coverage is 67.28%.

❗ Current head 5ed0c10 differs from pull request most recent head eb324a0. Consider uploading reports for the commit eb324a0 to get more accurate results

@@           Coverage Diff           @@
##           master   #19767   +/-   ##
=======================================
  Coverage   66.51%   66.51%           
=======================================
  Files        1687     1690    +3     
  Lines       64618    64616    -2     
  Branches     6646     6656   +10     
=======================================
  Hits        42978    42978           
+ Misses      19940    19937    -3     
- Partials     1700     1701    +1
Flag Coverage Δ
hive 52.69% <ø> (ø)
mysql 81.95% <ø> (ø)
postgres 82.00% <ø> (ø)
presto 52.54% <ø> (ø)
python 82.42% <ø> (ø)
sqlite 81.76% <ø> (ø)
unit 47.75% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset-frontend/src/components/Chart/Chart.jsx 52.54% <0.00%> (+2.54%) ⬆️
.../src/explore/components/ControlPanelsContainer.tsx 79.80% <ø> (ø)
superset/reports/api.py 89.84% <ø> (ø)
.../explore/components/ExploreViewContainer/index.jsx 52.29% <10.00%> (-2.60%) ⬇️
...perset-frontend/src/views/CRUD/alert/AlertList.tsx 65.30% <33.33%> (-1.37%) ⬇️
...frontend/src/components/ImportModal/ErrorAlert.tsx 40.00% <40.00%> (ø)
...rset-frontend/src/components/ImportModal/index.tsx 73.17% <50.00%> (+0.67%) ⬆️
...t-frontend/src/explore/components/ExploreAlert.tsx 58.33% <58.33%> (ø)
.../src/utils/getChartRequiredFieldsMissingMessage.ts 66.66% <66.66%> (ø)
...nd/src/explore/components/DataTablesPane/index.tsx 71.55% <79.31%> (-2.13%) ⬇️
... and 5 more

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 cf51459...eb324a0. Read the comment docs.

@eschutho eschutho added the risk:db-migration PRs that require a DB migration label Apr 18, 2022
eschutho
eschutho approved these changes Apr 18, 2022
View reviewed changes
Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

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

Looks good. Can you confirm in the description if the upgrade and downgrade have both been run?

@hughhhh
Copy link
Member Author

hughhhh commented Apr 19, 2022

Verifed locally that this migration works:

>> select count(*) from slices where query_context like '%time_range_endpoints%'
5

>> superset db upgrade
INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade cecc6bf46990 -> ad07e4fdbaba, rm_time_range_endpoints_from_qc_3
slices updated with no time_range_endpoints: 5

>> select count(*) from slices where query_context like '%time_range_endpoints%'
0

craig-rueda
craig-rueda approved these changes Apr 19, 2022
View reviewed changes
Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

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

Left a few minor comments

superset/migrations/versions/ad07e4fdbaba_rm_time_range_endpoints_from_qc_3.py
session.merge(slc)
slices_updated += 1

print(f"slices updated with no time_range_endpoints: {slices_updated}")
Copy link
Member

Choose a reason for hiding this comment

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

What if the update_slice method only returns a value if it actually finds the json key it's looking for? Then the log message would be accurate.

superset/migrations/versions/ad07e4fdbaba_rm_time_range_endpoints_from_qc_3.py
session.merge(slc)
slices_updated += 1

print(f"slices updated with no time_range_endpoints: {slices_updated}")
Copy link
Member

Choose a reason for hiding this comment

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

Please use logger.info instead of print.

Copy link
Member Author

Choose a reason for hiding this comment

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

logger.info wasn't properly printing when the running the upgrade

@hughhhh hughhhh merged commit 7e92340 into master Apr 19, 2022
sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Apr 19, 2022
@hughhhh @sadpandajoe
fix: Fix migration for removing time_range_endpoints 3 (apache#19767)
3b7fdf7 
* fix migration

* so dumb

* update test

* add code change

* retest

(cherry picked from commit 7e92340)
@sadpandajoe
Copy link
Member

🏷️ preset:2022.15

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@hughhhh @philipher29
fix: Fix migration for removing time_range_endpoints 3 (apache#19767)
06a584c 
* fix migration

* so dumb

* update test

* add code change

* retest
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
@mistercrunch mistercrunch deleted the time_range_endpoints_rm_qc_3 branch March 26, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eschutho eschutho eschutho approved these changes

@craig-rueda craig-rueda craig-rueda approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset:2022.15 risk:db-migration PRs that require a DB migration size/L 🚢 2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hughhhh @eschutho @sadpandajoe @craig-rueda @mistercrunch