john-bodley commented on code in PR #25147:
URL: https://github.com/apache/superset/pull/25147#discussion_r1364940555
##########
superset/common/query_context_factory.py:
##########
@@ -185,6 +185,7 @@ def _apply_granularity(
filter
for filter in query_object.filter
if filter["col"] != filter_to_remove
+ or filter["op"] != "TEMPORAL_RANGE"
Review Comment:
What is the reason for this change, i.e., why are we including a filter
which was flagged for removal if it operation is not a temporal range?
##########
superset/common/query_object_factory.py:
##########
@@ -102,3 +113,54 @@ def _process_row_limit(
# light version of the view.utils.core
# import view.utils require application context
# Todo: move it and the view.utils.core to utils package
+
+ def _process_filters(
+ self, datasource: BaseDatasource, query_filters:
list[QueryObjectFilterClause]
+ ) -> list[QueryObjectFilterClause]:
+ def get_dttm_filter_value(
+ value: Any, col: BaseColumn, date_format: str
+ ) -> int | str:
+ if not isinstance(value, int):
+ return value
+ if date_format in {"epoch_ms", "epoch_s"}:
+ if date_format == "epoch_s":
+ value = str(value)
+ else:
+ value = str(value * 1000)
+ else:
+ dttm = datetime.utcfromtimestamp(value / 1000)
Review Comment:
If the value is an integer and the date format isn't `epoch_ms` or `epoch`
what else could it be? It's not overly evident what this function is trying to
do given the return type is mixed.
##########
superset/common/query_object_factory.py:
##########
@@ -102,3 +113,54 @@ def _process_row_limit(
# light version of the view.utils.core
# import view.utils require application context
# Todo: move it and the view.utils.core to utils package
+
+ def _process_filters(
+ self, datasource: BaseDatasource, query_filters:
list[QueryObjectFilterClause]
+ ) -> list[QueryObjectFilterClause]:
+ def get_dttm_filter_value(
+ value: Any, col: BaseColumn, date_format: str
+ ) -> int | str:
+ if not isinstance(value, int):
+ return value
Review Comment:
If `value` is `Any` but not an `int` there's no guarantee that the return
type will be a `str`. Am I missing something here?
--
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]
