Re: [PR] feature(sqllab): Add cache sqllab sidebar table schema (#32796) [superset]

Mon, 24 Mar 2025 13:25:24 -0700 


korbit-ai[bot] commented on code in PR #32830:
URL: https://github.com/apache/superset/pull/32830#discussion_r2010874699


##########
superset/config.py:
##########
@@ -785,6 +785,9 @@ class D3TimeFormat(TypedDict, total=False):
 # Cache for datasource metadata and query results
 DATA_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "NullCache"}
 
+# Schema caching configuration
+SCHEMA_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "NullCache"}

Review Comment:
   ### Ineffective Schema Cache Configuration <sub>![category 
Performance](https://img.shields.io/badge/Performance-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### 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:
   ```python
   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
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d6002b1-89be-4cbd-a1dd-fb4d2d391898/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d6002b1-89be-4cbd-a1dd-fb4d2d391898?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d6002b1-89be-4cbd-a1dd-fb4d2d391898?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d6002b1-89be-4cbd-a1dd-fb4d2d391898?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d6002b1-89be-4cbd-a1dd-fb4d2d391898)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:5e094707-2528-4575-9c88-cc58c71d7c2b -->
   
   [](5e094707-2528-4575-9c88-cc58c71d7c2b)



##########
superset/databases/utils.py:
##########
@@ -71,6 +71,13 @@ def get_table_metadata(database: Any, table: Table) -> 
TableMetadataResponse:
     :param table: Table instance
     :return: Dict table metadata ready for API response
     """
+    # Lazy import to prevent circular dependency
+    from superset.extensions import cache_manager

Review Comment:
   ### Incomplete circular dependency comment <sub>![category 
Documentation](https://img.shields.io/badge/Documentation-7c3aed)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### 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
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f3fad8eb-bd22-454b-8b35-79213c8c99cf/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f3fad8eb-bd22-454b-8b35-79213c8c99cf?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f3fad8eb-bd22-454b-8b35-79213c8c99cf?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f3fad8eb-bd22-454b-8b35-79213c8c99cf?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f3fad8eb-bd22-454b-8b35-79213c8c99cf)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:7ca63084-52cf-4357-af62-aa799af0beb1 -->
   
   [](7ca63084-52cf-4357-af62-aa799af0beb1)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to