[GitHub] [incubator-superset] villebro commented on a change in pull request #11499: feat(SIP-39): Async query support for charts

[GitBox <gi...@apache.org>]

villebro commented on a change in pull request #11499:
URL: https://github.com/apache/incubator-superset/pull/11499#discussion_r533960090



##########
File path: superset/config.py
##########
@@ -326,6 +326,8 @@ def _try_json_readsha(  # pylint: disable=unused-argument
     "DISPLAY_MARKDOWN_HTML": True,
     # When True, this escapes HTML (rather than rendering it) in Markdown components
     "ESCAPE_MARKDOWN_HTML": False,
+    "SIP_34_ANNOTATIONS_UI": False,

Review comment:
       This should probably not be here

##########
File path: superset/common/query_context.py
##########
@@ -75,6 +75,13 @@ def __init__(  # pylint: disable=too-many-arguments
         self.custom_cache_timeout = custom_cache_timeout
         self.result_type = result_type or utils.ChartDataResultType.FULL
         self.result_format = result_format or utils.ChartDataResultFormat.JSON
+        self.cache_values = {
+            "datasource": datasource,
+            "queries": queries,
+            "force": force,
+            "result_type": result_type,
+            "result_format": result_format,

Review comment:
       Should these reference `self`, e.g. `"datasource": self.datasource`?

##########
File path: superset-frontend/src/chart/Chart.jsx
##########
@@ -157,6 +157,8 @@ class Chart extends React.PureComponent {
       queryResponse,
     } = this.props;
 
+    console.log('**** Chart renderError', queryResponse);
+

Review comment:
       Probably a leftover from debugging.
   ```suggestion
   ```

##########
File path: superset/common/query_context.py
##########
@@ -184,9 +194,30 @@ def get_single_payload(self, query_obj: QueryObject) -> Dict[str, Any]:
             return {"data": payload["data"]}
         return payload
 
-    def get_payload(self) -> List[Dict[str, Any]]:
-        """Get all the payloads from the QueryObjects"""
-        return [self.get_single_payload(query_object) for query_object in self.queries]
+    def get_payload(self, **kwargs: Any) -> Dict[str, Any]:
+        cache_query_context = (
+            kwargs["cache_query_context"] if "cache_query_context" in kwargs else False
+        )
+        force_cached = kwargs["force_cached"] if "force_cached" in kwargs else False

Review comment:
       Would `kwargs.get("cache_query_context", false)` work here? Same for `force_cached`. Alternatively, should we introduce `cache_query_context` and `force_cached` as explicitly named keyword arguments instead of via `kwargs`?

##########
File path: superset/common/query_context.py
##########
@@ -201,7 +232,18 @@ def cache_timeout(self) -> int:
             return self.datasource.database.cache_timeout
         return config["CACHE_DEFAULT_TIMEOUT"]
 
-    def cache_key(self, query_obj: QueryObject, **kwargs: Any) -> Optional[str]:
+    def cache_key(self, **extra: Any) -> str:
+        """
+        The cache key is made out of the key/values from self.cached_values, plus any
+        other key/values in `extra`
+        """
+        key_prefix = "qc-"
+        cache_dict = self.cache_values.copy()
+        cache_dict.update(extra)
+
+        return generate_cache_key(cache_dict, key_prefix)

Review comment:
       I believe this will cause unnecessary cache misses, as it's calculating the key based on the raw queries in `self.cache_values`, not `QueryObject.cache_key()`. Can we make it so that the `queries` property in `self.cache_values` is the respective `cache_key` for said `QueryObject`, not the full object?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org