Skip to content

fix: Update migration logic in #27119#28422

Merged
john-bodley merged 1 commit into
masterfrom
john-bodley--upgrade-migration-fcea065655
May 13, 2024
Merged

fix: Update migration logic in #27119#28422
john-bodley merged 1 commit into
masterfrom
john-bodley--upgrade-migration-fcea065655

Conversation

@john-bodley
Copy link
Copy Markdown
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
Copy Markdown
Member

mistercrunch commented May 10, 2024

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
Copy Markdown
Member

michael-s-molina commented May 10, 2024

Thanks for fixing that @john-bodley 🙏🏼

@michael-s-molina
Copy link
Copy Markdown
Member

michael-s-molina commented May 10, 2024

@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
Comment thread superset/models/sql_lab.py
schema = Column(String(256))
catalog = Column(String(256), nullable=True, default=None)
sql = Column(MediumText())
sql = Column(LongText())
Copy link
Copy Markdown
Member

@michael-s-molina michael-s-molina May 10, 2024

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
Copy Markdown
Member Author

@john-bodley john-bodley May 10, 2024

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
Copy Markdown
Member Author

john-bodley commented May 13, 2024

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
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
@mistercrunch mistercrunch added 🍒 4.0.2 Cherry-picked to 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
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@john-bodley
fix: Update migration logic in apache#27119 (apache#28422)
cd645a3
@mistercrunch mistercrunch added the 🚢 4.1.0 First shipped in 4.1.0 label Nov 27, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
@john-bodley
fix: Update migration logic in apache#27119 (apache#28422)
3f71b1e
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 Cherry-picked to 4.0.2 🚢 4.1.0 First shipped in 4.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@john-bodley @mistercrunch @michael-s-molina