-
Notifications
You must be signed in to change notification settings - Fork 16.6k
feat(sqllab): Add cache sqllab sidebar table schema (#32796) #32830
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
base: master
Are you sure you want to change the base?
Conversation
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 | Fix Detected |
|---|---|---|
| Incomplete circular dependency comment ▹ view | ||
| Ineffective Schema Cache Configuration ▹ view |
Suppressed issues based on your team's Korbit activity
| This issue | Is similar to | Because |
|---|---|---|
|
Cache key construction doesn't account for potential special characters in table or schema names that could cause key collisions |
Similar issues were not addressed in the past | |
|
The cache implementation lacks TTL (Time To Live) settings which could lead to stale data being served indefinitely. |
Similar issues were not addressed in the past |
When you react to issues (for example, an upvote or downvote) or you fix them, Korbit will tune future reviews based on these signals.
Files scanned
| File Path | Reviewed |
|---|---|
| superset/utils/cache_manager.py | ✅ |
| superset/databases/utils.py | ✅ |
| superset/config.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Documentation ✅ Logging ✅ Error Handling ✅ Readability ✅ Design ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
| # Lazy import to prevent circular dependency | ||
| from superset.extensions import cache_manager |
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.
Incomplete circular dependency comment 
Tell me more
What is the issue?
The inline comment about lazy import is too brief and doesn't explain which modules are involved in the circular dependency.
Why this matters
Future maintainers may inadvertently create import problems without understanding which modules are affected.
Suggested change ∙ Feature Preview
Lazy import to prevent circular dependency between superset.extensions and superset.databases modules
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| DATA_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "NullCache"} | ||
|
|
||
| # Schema caching configuration | ||
| SCHEMA_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "NullCache"} |
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.
Ineffective Schema Cache Configuration 
Tell me more
What is the issue?
The default schema cache configuration is set to use NullCache, which effectively disables caching.
Why this matters
The NullCache setting will prevent any actual caching from happening, defeating the purpose of implementing the schema caching mechanism for faster table metadata retrieval.
Suggested change ∙ Feature Preview
Set a default cache backend that actually performs caching, for example:
SCHEMA_CACHE_CONFIG: CacheConfig = {
"CACHE_TYPE": "RedisCache",
"CACHE_DEFAULT_TIMEOUT": 86400, # 24 hours
"CACHE_KEY_PREFIX": "schema_",
"CACHE_REDIS_URL": "redis://localhost:6379/1"
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #32830 +/- ##
===========================================
+ Coverage 60.48% 83.41% +22.92%
===========================================
Files 1931 549 -1382
Lines 76236 39476 -36760
Branches 8568 0 -8568
===========================================
- Hits 46114 32927 -13187
+ Misses 28017 6549 -21468
+ Partials 2105 0 -2105
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:
|
|
Oh right. Forgot about this button. So if that button adds a Extra points if you add a |
|
Makes sense. Let me work on adding a force refresh and latest_refreshed to the API |



SUMMARY
Add cache for table schema in sqllab.
Motivation: Fetching schema can be a slow operation on query engines such as Trino and Athena. We have noticed the schema takes up to 300s to appear when Trino cluster is under load. Caching the schema allows users to have better interactive experience with sqllab.
Design: Added a new schema cache to cache manager so this can be managed separately from other caches. Many users including relational DB's such as Postgres or MySQL likely won't require this feature as this feature is ideal for query engines where fetching table schema is not a trivial operation. Feature is disabled by default and requires setting SCHEMA_CACHE_CONFIG in config.py to be enable.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
[x] Required feature flags:
SCHEMA_CACHE_CONFIG = {
'CACHE_TYPE': 'redis',
'CACHE_DEFAULT_TIMEOUT': 60 * 60 * 12, # 12 hr cache
'CACHE_KEY_PREFIX': 'superset_schema_',
'CACHE_REDIS_URL': f"redis://{REDIS}:{REDIS_PORT}/{REDIS_DB}"
}