-
Notifications
You must be signed in to change notification settings - Fork 16.2k
fix: Migrate charts with empty query_context #33710
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 charts with empty query_context #33710
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #33710 +/- ##
===========================================
+ Coverage 60.48% 72.95% +12.46%
===========================================
Files 1931 558 -1373
Lines 76236 40282 -35954
Branches 8568 4222 -4346
===========================================
- Hits 46114 29387 -16727
+ Misses 28017 9804 -18213
+ Partials 2105 1091 -1014
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Missing Query Builder Implementation ▹ view | 🧠 Not in standard |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/migrations/shared/migrate_viz/base.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| queries = clz._build_query()["queries"] | ||
| query_context["queries"] = queries | ||
| else: | ||
| query_context = clz._build_query() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Was this a blocker for 5.0? Perhaps the only block RN!? |
This was one of the blockers from our side. I'm not sure if it's the only blocker because people didn't vote on RC3 yet 🤷🏼♂️ |
(cherry picked from commit 08655a7)

SUMMARY
This PR implements a fix for the legacy charts migration. When a chart has a
nullquery_context, a newquery_contextwill be generated using the migratedform_data. Additionally, the query backup will be saved asnullin the backup fieldqueries_bak, ensuring that if the chart is downgraded, thequery_contextwill revert tonull.This change was necessary because executing
warm_up_cachefor a migrated chart with anullquery_contextresults in the error:Chart's query context does not exist. As a consequence, the chart fails to be cached.TESTING INSTRUCTIONS
Run superset db upgrade and superset db downgrade CLI commands.
ADDITIONAL INFORMATION