rusackas commented on code in PR #41489:
URL: https://github.com/apache/superset/pull/41489#discussion_r3488872472
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -2849,6 +2859,108 @@ def test_normalize_df_normalizes_legacy_time_column()
-> None:
assert result[DTTM_ALIAS][0].strftime("%Y-%m-%d") == "2020-01-01"
+def test_normalize_df_applies_offset_to_all_temporal_columns() -> None:
+ """Regression test for issue #23167: the dataset HOURS OFFSET must be
applied
+ to every temporal column a query returns, not only the selected time
column.
+ Two native-datetime temporal columns (neither declaring a
+ ``python_date_format``) must both be shifted by the dataset offset."""
+ import pandas as pd
+
+ from superset.models.helpers import ExploreMixin
+
+ created = MagicMock(
+ column_name="created",
+ is_dttm=True,
+ python_date_format=None,
+ datetime_format=None,
+ )
+ expired = MagicMock(
+ column_name="expired",
+ is_dttm=True,
+ python_date_format=None,
+ datetime_format=None,
+ )
+ columns = {"created": created, "expired": expired}
+
+ datasource = MagicMock()
+ datasource.offset = 4
+ datasource.enforce_numerical_metrics = False
+ datasource.columns = list(columns.values())
+ datasource.get_column = lambda name: columns.get(name)
Review Comment:
This is a one-line mock in test setup... I think the lambda reads cleaner
than a typed `def` here, and mypy is happy with it, so I will leave it.
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -2849,6 +2859,108 @@ def test_normalize_df_normalizes_legacy_time_column()
-> None:
assert result[DTTM_ALIAS][0].strftime("%Y-%m-%d") == "2020-01-01"
+def test_normalize_df_applies_offset_to_all_temporal_columns() -> None:
+ """Regression test for issue #23167: the dataset HOURS OFFSET must be
applied
+ to every temporal column a query returns, not only the selected time
column.
+ Two native-datetime temporal columns (neither declaring a
+ ``python_date_format``) must both be shifted by the dataset offset."""
+ import pandas as pd
+
+ from superset.models.helpers import ExploreMixin
+
+ created = MagicMock(
+ column_name="created",
+ is_dttm=True,
+ python_date_format=None,
+ datetime_format=None,
+ )
+ expired = MagicMock(
+ column_name="expired",
+ is_dttm=True,
+ python_date_format=None,
+ datetime_format=None,
+ )
+ columns = {"created": created, "expired": expired}
+
+ datasource = MagicMock()
+ datasource.offset = 4
+ datasource.enforce_numerical_metrics = False
+ datasource.columns = list(columns.values())
+ datasource.get_column = lambda name: columns.get(name)
+ for method in (
+ "_python_date_format",
+ "_collect_dttm_labels",
+ "_offset_only_dttm_cols",
+ "normalize_df",
+ ):
+ setattr(datasource, method, getattr(ExploreMixin,
method).__get__(datasource))
+
+ query_object = MagicMock()
+ query_object.columns = ["created", "expired"]
+ query_object.granularity = None
+ query_object.time_shift = None
+
+ df = pd.DataFrame(
+ {
+ "created": pd.to_datetime(["2020-01-01 00:00:00", "2020-01-02
00:00:00"]),
+ "expired": pd.to_datetime(["2020-06-01 12:00:00", "2020-06-02
12:00:00"]),
+ }
+ )
+
+ result = datasource.normalize_df(df, query_object)
+
+ assert (
+ result["created"].tolist()
+ == pd.to_datetime(["2020-01-01 04:00:00", "2020-01-02
04:00:00"]).tolist()
+ )
+ assert (
+ result["expired"].tolist()
+ == pd.to_datetime(["2020-06-01 16:00:00", "2020-06-02
16:00:00"]).tolist()
+ )
+
+
+def test_normalize_df_offset_skips_unconfigured_integer_temporal_columns() ->
None:
+ """The offset extension for native-datetime temporal columns must not
touch a
+ temporal column whose values arrive as plain integers with no declared
+ format: such a column cannot be safely interpreted as a datetime, so it is
+ left untouched rather than reinterpreted as nanoseconds (see issue
#23167)."""
+ import pandas as pd
+ from pandas.api.types import is_datetime64_any_dtype
+
+ from superset.models.helpers import ExploreMixin
+
+ int_col = MagicMock(
+ column_name="ts",
+ is_dttm=True,
+ python_date_format=None,
+ datetime_format=None,
+ )
+ datasource = MagicMock()
+ datasource.offset = 4
+ datasource.enforce_numerical_metrics = False
+ datasource.columns = [int_col]
+ datasource.get_column = lambda name: {"ts": int_col}.get(name)
Review Comment:
Same as the other one... it is a throwaway mock in the test, so a typed
`def` would just be noise 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]
