mikebridge commented on code in PR #42052:
URL: https://github.com/apache/superset/pull/42052#discussion_r3589004920
##########
superset/common/query_context_factory.py:
##########
@@ -245,6 +245,20 @@ def _apply_granularity( # noqa: C901
}
x_axis = form_data and form_data.get("x_axis")
+ should_infer_filter_granularity = (
+ is_adhoc_column(x_axis) # type: ignore
+ and query_object.granularity is None
+ and bool(query_object.from_dttm or query_object.to_dttm)
+ and (main_dttm_col := getattr(datasource, "main_dttm_col", None))
+ in temporal_columns
+ )
Review Comment:
Good catch—addressed in 06abb2e194. Inference now distinguishes a scalar
`query_object.time_range` from bounds derived solely from an existing
`TEMPORAL_RANGE`. A lone temporal filter on another datetime column no longer
causes `main_dttm_col` to be inferred. The dashboard-range plus
independent-filter case remains supported, and a regression test covers both
cases.
##########
tests/unit_tests/models/test_no_filter_time_range.py:
##########
@@ -208,6 +210,43 @@ def test_actual_from_to_dttm_produces_time_filter(
)
+def test_time_range_without_granularity_filters_main_datetime_column(
+ mocker: MockerFixture,
+ app: Flask,
+) -> None:
+ """A SQL-expression dimension must not cause a dashboard range to be
dropped."""
+ dataset = _make_dataset(mocker)
+ expression: AdhocColumn = {
+ "sqlExpression": "value / 10",
+ "label": "value_bucket",
+ "columnType": "BASE_AXIS",
+ }
+ query_object = QueryObject(
+ granularity=None,
+ is_timeseries=False,
+ metrics=["count"],
+ columns=[expression],
+ from_dttm=datetime(2024, 1, 1, tzinfo=timezone.utc),
+ to_dttm=datetime(2024, 1, 31, tzinfo=timezone.utc),
+ )
Review Comment:
Addressed in 06abb2e194: the new `QueryObject` local now has an explicit
annotation.
##########
tests/unit_tests/common/test_query_context_factory.py:
##########
@@ -384,6 +387,94 @@ def test_apply_granularity_no_granularity(self):
assert query_object.columns == ["ds", "other_col"]
+ def
test_apply_granularity_uses_main_datetime_for_bounded_expression_axis(self):
Review Comment:
Addressed in 06abb2e194: the new test methods now have explicit `-> None`
return annotations.
##########
superset/common/query_context_factory.py:
##########
@@ -245,6 +245,20 @@ def _apply_granularity( # noqa: C901
}
x_axis = form_data and form_data.get("x_axis")
+ should_infer_filter_granularity = (
+ is_adhoc_column(x_axis) # type: ignore
+ and query_object.granularity is None
+ and bool(query_object.from_dttm or query_object.to_dttm)
+ and (main_dttm_col := getattr(datasource, "main_dttm_col", None))
+ in temporal_columns
+ )
Review Comment:
Addressed in 06abb2e194: the new boolean local now has an explicit `bool`
annotation.
--
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]
