[GitHub] [superset] mistercrunch commented on a change in pull request #15279: feat: run extra query on QueryObject and add compare operator for post_processing

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

mistercrunch commented on a change in pull request #15279:
URL: https://github.com/apache/superset/pull/15279#discussion_r666303786



##########
File path: superset/utils/date_parser.py
##########
@@ -106,6 +119,16 @@ def dttm_from_timetuple(date_: struct_time) -> datetime:
     )
 
 
+def get_past_or_future(
+    human_readable: Optional[str], source_time: Optional[datetime] = None,
+) -> datetime:
+    cal = parsedatetime.Calendar()

Review comment:
       The next few lines have a lot in common with `parse_human_timedelta`, should we refactor them both use a common simple method?

##########
File path: superset/charts/commands/exceptions.py
##########
@@ -56,6 +56,23 @@ def __init__(self, human_readable: str) -> None:
         )
 
 
+class TimeDeltaUnclearError(ValidationError):
+    """
+    Time delta is in valid error.

Review comment:
       typo "invalid", better use "unclear" for consistency 

##########
File path: superset/common/utils.py
##########
@@ -0,0 +1,162 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+from typing import Any, Dict, Optional
+
+from flask_caching import Cache
+from pandas import DataFrame
+
+from superset import app
+from superset.constants import CacheRegion
+from superset.exceptions import CacheLoadError
+from superset.extensions import cache_manager
+from superset.models.helpers import QueryResult
+from superset.stats_logger import BaseStatsLogger
+from superset.utils.cache import set_and_log_cache
+from superset.utils.core import error_msg_from_exception, get_stacktrace, QueryStatus
+
+config = app.config
+stats_logger: BaseStatsLogger = config["STATS_LOGGER"]
+logger = logging.getLogger(__name__)
+
+_cache: Dict[CacheRegion, Cache] = {
+    CacheRegion.DEFAULT: cache_manager.cache,
+    CacheRegion.DATA: cache_manager.data_cache,
+}
+
+
+class QueryCacheManager:

Review comment:
       docstring pls, I'm not sure what exactly this is for

##########
File path: superset/common/query_context.py
##########
@@ -101,21 +104,143 @@ def __init__(  # pylint: disable=too-many-arguments
             "result_format": self.result_format,
         }
 
-    def get_query_result(self, query_object: QueryObject) -> Dict[str, Any]:
-        """Returns a pandas dataframe based on the query object"""
+    @staticmethod
+    def left_join_on_dttm(
+        left_df: pd.DataFrame, right_df: pd.DataFrame
+    ) -> pd.DataFrame:
+        df = left_df.set_index(DTTM_ALIAS).join(right_df.set_index(DTTM_ALIAS))
+        df.reset_index(level=0, inplace=True)
+        return df
+
+    def processing_time_offsets(

Review comment:
       NIT: looking at the function signature, personally I'm not big on methods that return a tuple of mixed objects, it feels like maybe we should be mutating the object this method is part of, or returning a more complex object with a proper name "CachedDataFrame" or something (unclear to me at this point what the best name would be). I namedtuple could be a good compromise.
   
   I'm not sure what it implies as I'm just starting to review this code




-- 
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: notifications-unsubscribe@superset.apache.org

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