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: Update migration logic in #27119 #28422

Merged
merged 1 commit into from
May 13, 2024
Merged

fix: Update migration logic in #27119 #28422

merged 1 commit into from
May 13, 2024

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented May 10, 2024 •
edited
Loading

SUMMARY

I realize it's generally perceived as poor form to change an existing migration, however per here the query. executed_sql and query. select_sql were previously defined as type LONGTEXT rather than TEXT, thus #27119 resulted in:

  1. Downsizing these columns to MEDIUMTEXT could result in data loss.
  2. Altering the query table unnecessarily can be rather expensive given the size of the table.

This PR remove mutating these columns unnecessarily.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Tested the migration using a MySQL metadata database.

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

@github-actions github-actions bot added the risk:db-migration PRs that require a DB migration label May 10, 2024
@john-bodley john-bodley force-pushed the john-bodley--upgrade-migration-fcea065655 branch from 7be1b00 to f08975a Compare May 10, 2024 04:00
@john-bodley john-bodley marked this pull request as ready for review May 10, 2024 04:31
@john-bodley john-bodley requested a review from a team as a code owner May 10, 2024 04:31
@mistercrunch mistercrunch added the risk:may-require-downtime Upgrading to this feature may require downtime label May 10, 2024
@mistercrunch
Copy link
Member

Tagging as "may require downtime" knowing that mysql takes table-level lock for this type of operation and that this table tend to get large, and is busy with polling (sqllab client poking at query status).

@john-bodley john-bodley changed the title fix: Update logic in migration fcea065655 fix: Update migration logic in #27119 May 10, 2024
@john-bodley john-bodley force-pushed the john-bodley--upgrade-migration-fcea065655 branch from f08975a to 91ceda7 Compare May 10, 2024 07:01
@michael-s-molina
Copy link
Member

Thanks for fixing that @john-bodley 🙏🏼

@michael-s-molina
Copy link
Member

@john-bodley Before merging this PR, can you do the additional improvement of adding a LongText type similar to MediumText? I believe this error was caused because the models were defined as Text so you could only see the real type in the migration scripts. This type or error can be avoided if we have LongText.

@john-bodley john-bodley force-pushed the john-bodley--upgrade-migration-fcea065655 branch from 91ceda7 to 5de9bea Compare May 10, 2024 14:21
michael-s-molina
michael-s-molina reviewed May 10, 2024
View reviewed changes
superset/models/sql_lab.py
@@ -110,11 +116,11 @@ class Query(
sql_editor_id = Column(String(256), index=True)
schema = Column(String(256))
catalog = Column(String(256), nullable=True, default=None)
sql = Column(MediumText())
sql = Column(LongText())
Copy link
Member

Choose a reason for hiding this comment

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

@john-bodley Just to confirm.. is sql LongText or MediumText? The reason I'm asking is because it's not being affected by your script.

Copy link
Member Author

Choose a reason for hiding this comment

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

Per here sql is LongText and isn't impacted by any of the aforementioned migrations.

@john-bodley john-bodley force-pushed the john-bodley--upgrade-migration-fcea065655 branch from 5de9bea to 2fdb734 Compare May 10, 2024 20:18
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 10, 2024
@john-bodley john-bodley force-pushed the john-bodley--upgrade-migration-fcea065655 branch 3 times, most recently from b3809d7 to 1bd4679 Compare May 11, 2024 00:48
@john-bodley john-bodley force-pushed the john-bodley--upgrade-migration-fcea065655 branch from 1bd4679 to 67a0522 Compare May 13, 2024 16:19
@john-bodley john-bodley force-pushed the john-bodley--upgrade-migration-fcea065655 branch from 67a0522 to 2fbc926 Compare May 13, 2024 18:05
@john-bodley
Copy link
Member Author

Given we can't cherry-pick new migrations, I thought it would be prudent to break this PR into two,

  1. This PR which "fixes" the existing migration by preventing the downsizing.
  2. fix: Update migration logic in #27119 #28482 which adds a new migration to resizes any wrongfully downsized columns.

as that way we can cherry-pick (1) into the 4.0 branch.

@john-bodley john-bodley removed risk:db-migration PRs that require a DB migration risk:may-require-downtime Upgrading to this feature may require downtime labels May 13, 2024
@michael-s-molina michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label May 13, 2024
@john-bodley john-bodley mentioned this pull request May 13, 2024
Merged
9 tasks
@john-bodley john-bodley merged commit 1ccbc65 into master May 13, 2024
35 checks passed
michael-s-molina pushed a commit that referenced this pull request May 13, 2024
@john-bodley @michael-s-molina
fix: Update migration logic in #27119 (#28422)
804ecf4
aehanno pushed a commit to kosmos-education/superset that referenced this pull request May 13, 2024
@john-bodley @aehanno
fix: Update migration logic in apache#27119 (apache#28422)
7b2fa7f
michael-s-molina pushed a commit that referenced this pull request May 13, 2024
@john-bodley @michael-s-molina
fix: Update migration logic in #27119 (#28422)
78d5b47
@john-bodley john-bodley deleted the john-bodley--upgrade-migration-fcea065655 branch May 13, 2024 20:24
jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024
@john-bodley @jzhao62
fix: Update migration logic in apache#27119 (apache#28422)
b648e43
EnxDev pushed a commit to EnxDev/superset that referenced this pull request May 31, 2024
@john-bodley @EnxDev
fix: Update migration logic in apache#27119 (apache#28422)
8fe25f3
@mistercrunch mistercrunch added 🍒 4.0.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-s-molina michael-s-molina michael-s-molina approved these changes

@eschutho eschutho Awaiting requested review from eschutho eschutho is a code owner

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch 🍒 4.0.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@john-bodley @mistercrunch @michael-s-molina