[GitHub] [superset] villebro commented on a diff in pull request #24176: fix: Time shifts with different granularity for ECharts

["villebro (via GitHub)" <gi...@apache.org>]

villebro commented on code in PR #24176:
URL: https://github.com/apache/superset/pull/24176#discussion_r1217949437


##########
superset/common/query_context_processor.py:
##########
@@ -328,7 +378,25 @@ def processing_time_offsets(  # pylint: disable=too-many-locals,too-many-stateme
                     "when using a Time Comparison."
                 )
             )
-        for offset in time_offsets:
+
+        columns = df.columns
+        time_grain = self.get_time_grain(query_context, query_object) or TimeGrain.DAY
+        join_column_producer = config["TIME_GRAIN_JOIN_COLUMN_PRODUCERS"].get(
+            time_grain
+        )
+        use_aggregated_join_column = (
+            time_grain in AGGREGATED_JOIN_GRAINS or join_column_producer
+        )

Review Comment:
   Ok, this is a total mininit and functionally they're equivalent, but for readability I'd place the check for `join_column_producer` first, as it takes precedent in downstream logic (this code is generally pretty heavy on the eyes, so any improved readability helps):
   ```suggestion
           use_aggregated_join_column = (
               join_column_producer  or time_grain in AGGREGATED_JOIN_GRAINS
           )
   ```



##########
tests/unit_tests/common/test_get_aggregated_join_column.py:
##########
@@ -0,0 +1,52 @@
+# 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.
+from pandas import Timestamp
+
+from superset.common.query_context_processor import QueryContextProcessor
+from superset.constants import TimeGrain
+
+get_aggregated_join_column = QueryContextProcessor.get_aggregated_join_column
+
+row = [Timestamp("2020-01-07")]
+
+
+def test_week_join_column():

Review Comment:
   I notice we don't have any tests for `TIME_GRAIN_JOIN_COLUMN_PRODUCERS` - it'd be nice to add at least one test to guard against future regressions.



##########
superset/common/query_context_processor.py:
##########
@@ -74,6 +74,27 @@
 stats_logger: BaseStatsLogger = config["STATS_LOGGER"]
 logger = logging.getLogger(__name__)
 
+# Artificial column used for joining aggregated offset results
+AGGREGATED_JOIN_COLUMN = "__aggregated_join_column"

Review Comment:
   nit: temporary seems more appropriate here
   ```suggestion
   # Temporary column used for joining aggregated offset results
   AGGREGATED_JOIN_COLUMN = "__aggregated_join_column"
   ```



##########
superset/common/query_context_processor.py:
##########
@@ -328,7 +378,25 @@ def processing_time_offsets(  # pylint: disable=too-many-locals,too-many-stateme
                     "when using a Time Comparison."
                 )
             )
-        for offset in time_offsets:
+
+        columns = df.columns
+        time_grain = self.get_time_grain(query_context, query_object) or TimeGrain.DAY

Review Comment:
   Why are we defaulting to a daily time grain? If it's mandatory, I'd rather return a 400 if it's missing.



##########
superset/db_engine_specs/base.py:
##########
@@ -80,25 +81,25 @@ class TimeGrain(NamedTuple):
 
 
 builtin_time_grains: dict[str | None, str] = {
-    "PT1S": __("Second"),

Review Comment:
   The type declaration on line 82 should be updated; the key should be `TimeGrainConstants`, right?



##########
superset/common/query_context_processor.py:
##########
@@ -307,6 +324,40 @@ def _get_timestamp_format(
 
         return df
 
+    @staticmethod
+    def get_time_grain(
+        query_context: QueryContext, query_object: QueryObject
+    ) -> Any | None:
+        if query_context.form_data:
+            return query_context.form_data.get("time_grain_sqla")

Review Comment:
   I think this should be avoided. The form data has been introduced fairly recently in query_context, and isn't really meant to influence rendering of the query_object (IIRC, it was originally added to relay some context about query timeouts that are defined in the chart object). Is there some reason we are primarily looking at the form data here, and not the query 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.

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