[GitHub] [incubator-superset] villebro commented on a change in pull request #7131: Make timestamp expression native SQLAlchemy element

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

villebro commented on a change in pull request #7131: Make timestamp expression native SQLAlchemy element
URL: https://github.com/apache/incubator-superset/pull/7131#discussion_r280007860
 
 

 ##########
 File path: superset/db_engine_specs.py
 ##########
 @@ -105,28 +125,43 @@ class BaseEngineSpec(object):
     """Abstract class for database engine specific configurations"""
 
     engine = 'base'  # str as defined in sqlalchemy.engine.engine
-    time_grain_functions = {}
+    time_grain_functions: Dict[Optional[str], str] = {}
     time_groupby_inline = False
     limit_method = LimitMethod.FORCE_LIMIT
     time_secondary_columns = False
     inner_joins = True
     allows_subquery = True
     supports_column_aliases = True
     force_column_alias_quotes = False
-    arraysize = None
-    max_column_name_length = None
+    arraysize: Optional[int] = None
+    max_column_name_length: Optional[int] = None
 
     @classmethod
-    def get_time_expr(cls, expr, pdf, time_grain, grain):
+    def get_timestamp_expr(cls, col: ColumnClause, pdf: Optional[str],
+                           time_grain: Optional[str]) -> TimestampExpression:
+        """
+        Construct a TimeExpression to be used in a SQLAlchemy query.
+
+        :param col: Target column for the TimeExpression
+        :param pdf: date format (seconds or milliseconds)
+        :param time_grain: time grain, e.g. P1Y for 1 year
+        :return: TimestampExpression object
+        """
+        if time_grain:
+            time_expr = cls.time_grain_functions.get(time_grain)
+            if not time_expr:
+                raise NotImplementedError(
+                    f'No grain spec for {time_grain} for database {cls.engine}')
+        else:
+            time_expr = '{col}'
+
         # if epoch, translate to DATE using db specific conf
         if pdf == 'epoch_s':
-            expr = cls.epoch_to_dttm().format(col=expr)
+            time_expr = time_expr.replace('{col}', cls.epoch_to_dttm())
 
 Review comment:
   Previously the code conditionally "burned" the column name/expression into the epoch expression (later that gets conditionally "burned" into the timegrain expression), which is what this PR tries to avoid. In this proposal the epoch expression is instead conditionally burned into the timestamp expression, making it possible to keep the column object in it's native form until compilation time. I can change the `.replace` logic to `.format`, but in this context I don't think it will make a difference.

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


With regards,
Apache Git Services

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