sadpandajoe commented on code in PR #42054:
URL: https://github.com/apache/superset/pull/42054#discussion_r3593001552
##########
superset/utils/date_parser.py:
##########
@@ -66,6 +66,14 @@
"1st": 1,
}
+# parsedatetime does not understand "N quarters" (it leaves the source time
+# unchanged), so such phrases are rewritten to the equivalent number of months
+# before parsing. The lookbehind and the bounded repetition keep matching
+# linear on user-provided strings (every suffix of an unbounded digit run
+# would be re-scanned) and keep the int() conversion small; longer digit
+# runs fall through to parsedatetime like any other unparseable phrase.
+_QUARTERS_PATTERN = re.compile(r"(?<![0-9])([0-9]{1,10})\s+quarters?\b",
re.IGNORECASE)
Review Comment:
Annotated: `_QUARTERS_PATTERN: re.Pattern[str]` in a5b06005ba.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
tests/unit_tests/common/test_query_context_processor.py:
##########
@@ -1024,6 +1024,159 @@ def fake_query(dct: dict[str, Any]) -> MagicMock:
assert "2025-06-01" in val, f"Expected shifted-to-dttm in val, got:
{val!r}"
+def test_processing_time_offsets_quarter_offset_shifts_query_window(processor):
Review Comment:
Annotated `processor: QueryContextProcessor` and `-> None` in a5b06005ba,
with a typed `Any` alias for the MagicMock datasource the test binds real
methods onto.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
tests/unit_tests/common/test_query_context_processor.py:
##########
@@ -1024,6 +1024,159 @@ def fake_query(dct: dict[str, Any]) -> MagicMock:
assert "2025-06-01" in val, f"Expected shifted-to-dttm in val, got:
{val!r}"
+def test_processing_time_offsets_quarter_offset_shifts_query_window(processor):
+ """A quarter offset must shift the offset query's window, not just the
+ join keys. parsedatetime does not understand "1 quarter ago", so without
+ rewriting quarters to months the offset subquery silently runs against
+ the current period and the comparison series joins to nulls. The fake
+ query below derives its rows from the requested window, so the join only
+ yields the expected values when the window was actually shifted.
+ """
+ from superset.common.query_object import QueryObject
+ from superset.models.helpers import ExploreMixin
+
+ for method in (
+ "processing_time_offsets",
+ "_align_offset_without_time_grain",
+ "_coalesce_offset_index",
+ ):
+ setattr(
+ processor._qc_datasource,
+ method,
+ getattr(ExploreMixin, method).__get__(processor._qc_datasource),
+ )
+
+ df = pd.DataFrame(
+ {
+ "__timestamp": pd.to_datetime(["2024-04-01", "2024-05-01",
"2024-06-01"]),
+ "sum__num": [100, 200, 300],
+ }
+ )
+
+ query_object = QueryObject(
+ datasource=MagicMock(),
+ granularity="ds",
+ columns=[],
+ metrics=["sum__num"],
+ is_timeseries=True,
+ time_offsets=["1 quarter ago"],
+ filters=[
+ {
+ "col": "ds",
+ "op": "TEMPORAL_RANGE",
+ "val": "2024-04-01 : 2024-07-01",
+ }
+ ],
+ )
+
+ captured: list[dict[str, Any]] = []
+
+ def fake_query(dct: dict[str, Any]) -> MagicMock:
+ captured.append(dct)
+ result = MagicMock()
+ result.df = pd.DataFrame(
+ {
+ "__timestamp": pd.date_range(
+ start=dct["from_dttm"], periods=3, freq="MS"
+ ),
+ "sum__num": [1.0, 2.0, 3.0],
+ }
+ )
+ result.query = "SELECT 1"
+ return result
+
+ processor._qc_datasource.query = fake_query
+ processor._qc_datasource.normalize_df = MagicMock(
+ side_effect=lambda offset_df, _query_object: offset_df
+ )
+
+ with (
+ patch(
+ "superset.models.helpers.get_since_until_from_query_object",
+ return_value=(pd.Timestamp("2024-04-01"),
pd.Timestamp("2024-07-01")),
+ ),
+ patch(
+ "superset.common.utils.query_cache_manager.QueryCacheManager"
+ ) as mock_cache_manager,
+ patch.object(
+ processor._qc_datasource,
+ "get_time_grain",
+ return_value=None,
+ ),
+ ):
+ mock_cache = MagicMock()
+ mock_cache.is_loaded = False
+ mock_cache_manager.get.return_value = mock_cache
+
+ result = processor._qc_datasource.processing_time_offsets(
+ df, query_object, None, None, False
+ )
+
+ assert len(captured) == 1
+ assert captured[0]["from_dttm"] == pd.Timestamp("2024-01-01")
+ assert captured[0]["to_dttm"] == pd.Timestamp("2024-04-01")
+ assert result["df"]["sum__num__1 quarter ago"].tolist() == [1.0, 2.0, 3.0]
+
+
+def test_processing_time_offsets_rejects_unparseable_offset(processor):
Review Comment:
Annotated `processor: QueryContextProcessor` and `-> None` in a5b06005ba,
with a typed `Any` alias for the MagicMock datasource the test binds real
methods onto.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
tests/unit_tests/common/test_time_shifts.py:
##########
@@ -334,19 +337,259 @@ def test_join_offset_dfs_totals_query_no_dimensions():
assert_frame_equal(expected, result)
-def test_join_offset_dfs_raises_without_time_grain():
- """Time comparison with relative offsets requires a time grain."""
+def test_join_offset_dfs_no_time_grain_aligns_relative_offset() -> None:
+ """
+ Without a time grain, a relative offset joins on the exact shifted
+ timestamps instead of raising, so saved charts without a grain render
+ with a correctly aligned comparison series.
+ """
+ df = DataFrame(
+ {
+ "ds": [Timestamp("2021-01-01"), Timestamp("2021-02-01")],
+ "D": [1, 2],
+ }
+ )
+ offset_df = DataFrame(
+ {
+ "ds": [Timestamp("2020-01-01"), Timestamp("2020-02-01")],
+ "B": [5, 6],
+ }
+ )
+ offset_dfs = {"1 year ago": offset_df}
+
+ expected = DataFrame(
+ {
+ "ds": [Timestamp("2021-01-01"), Timestamp("2021-02-01")],
+ "D": [1, 2],
+ "B": [5, 6],
+ }
+ )
+
+ result = query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["ds"]
+ )
+
+ assert_frame_equal(expected, result)
+
+
+def test_join_offset_dfs_no_time_grain_unmatched_timestamps_yield_nulls() ->
None:
+ """
+ Without a time grain, offset timestamps that have no exact shifted
+ counterpart in the main series produce nulls instead of raising.
+ """
+ df = DataFrame({"ds": [Timestamp("2021-01-01")], "D": [1]})
+ offset_df = DataFrame({"ds": [Timestamp("2020-06-15")], "B": [5]})
+ offset_dfs = {"1 year ago": offset_df}
+
+ result = query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["ds"]
+ )
+
+ assert "B" in result.columns
+ assert result["B"].isna().all()
+
+
+def test_join_offset_dfs_no_time_grain_multiple_offsets() -> None:
+ """
+ Multiple relative offsets without a time grain each align on their own
+ shifted timestamps, and no synthetic join columns leak into the result.
+ """
+ df = DataFrame({"ds": [Timestamp("2021-01-29")], "D": [1]})
+ offset_df1 = DataFrame({"ds": [Timestamp("2021-01-01")], "B": [5]})
+ offset_df2 = DataFrame({"ds": [Timestamp("2020-01-29")], "C": [7]})
+ offset_dfs = {"28 days ago": offset_df1, "1 year ago": offset_df2}
+
+ expected = DataFrame(
+ {
+ "ds": [Timestamp("2021-01-29")],
+ "D": [1],
+ "B": [5],
+ "C": [7],
+ }
+ )
+
+ result = query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["ds"]
+ )
+
+ assert_frame_equal(expected, result)
+
+
+def test_join_offset_dfs_no_time_grain_empty_offset_df() -> None:
+ """
+ An empty offset series materializes its join keys as NaN floats; the
+ grain-less join must not crash on the dtype mismatch.
+ """
df = DataFrame({"ds": [Timestamp("2021-01-01")], "D": [1]})
- offset_df = DataFrame({"ds": [Timestamp("2021-02-01")], "B": [5]})
+ offset_df = DataFrame({"ds": [float("nan")], "B": [float("nan")]})
offset_dfs = {"1 year ago": offset_df}
- with pytest.raises(
- QueryObjectValidationError, match="Time Grain must be specified"
- ):
- query_context_processor.join_offset_dfs(
+ result = query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["ds"]
+ )
+
+ assert "B" in result.columns
+ assert result["B"].isna().all()
+
+
+def test_join_offset_dfs_no_time_grain_quarter_offset() -> None:
+ """
+ Quarter offsets align without a time grain (normalize_time_delta converts
+ quarters to months, since pd.DateOffset has no quarters argument).
+ """
+ df = DataFrame({"ds": [Timestamp("2021-04-01")], "D": [1]})
+ offset_df = DataFrame({"ds": [Timestamp("2021-01-01")], "B": [5]})
+ offset_dfs = {"1 quarter ago": offset_df}
+
+ expected = DataFrame({"ds": [Timestamp("2021-04-01")], "D": [1], "B": [5]})
+
+ result = query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["ds"]
+ )
+
+ assert_frame_equal(expected, result)
+
+
+def test_join_offset_dfs_no_time_grain_free_form_offset() -> None:
+ """
+ Offsets outside the normalize_time_delta grammar (e.g. "one year ago")
+ are aligned with the same parser that shifted the offset query's range.
+ """
+ df = DataFrame({"ds": [Timestamp("2021-01-01")], "D": [1]})
+ offset_df = DataFrame({"ds": [Timestamp("2020-01-01")], "B": [5]})
+ offset_dfs = {"one year ago": offset_df}
+
+ expected = DataFrame({"ds": [Timestamp("2021-01-01")], "D": [1], "B": [5]})
+
+ result = query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["ds"]
+ )
+
+ assert_frame_equal(expected, result)
+
+
+def test_join_offset_dfs_no_time_grain_free_form_offset_tz_microseconds() ->
None:
+ """
+ Free-form offsets align timezone-aware, sub-second timestamps: each row's
+ shift is computed from a naive second-truncated copy and applied to the
+ original value, so the join keys keep their timezone and precision.
+ """
+ df = DataFrame(
+ {"ds": [Timestamp("2021-03-04 05:06:07.890123", tz="UTC")], "D": [1]}
+ )
+ offset_df = DataFrame(
+ {"ds": [Timestamp("2020-03-04 05:06:07.890123", tz="UTC")], "B": [5]}
+ )
+ offset_dfs = {"one year ago": offset_df}
+
+ result = query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["ds"]
+ )
+
+ assert result["B"].tolist() == [5]
+
+
+def test_join_offset_dfs_no_time_grain_uninterpretable_offset() -> None:
+ """
+ An offset that no parser can interpret falls back to joining on the raw
+ keys (an empty comparison series) instead of crashing.
+ """
+ df = DataFrame({"ds": [Timestamp("2021-01-01")], "D": [1]})
+ offset_df = DataFrame({"ds": [Timestamp("2020-01-01")], "B": [5]})
+ offset_dfs = {"not a real offset": offset_df}
+
+ result = query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["ds"]
+ )
+
+ assert "B" in result.columns
+ assert result["B"].isna().all()
+
+
+def test_join_offset_dfs_no_time_grain_uninterpretable_offset_subsecond(
+ caplog,
+) -> None:
Review Comment:
Annotated `caplog: LogCaptureFixture` in a5b06005ba.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
superset/models/helpers.py:
##########
@@ -2369,31 +2483,13 @@ def join_offset_dfs(
time range instead of being truncated to the main series' range.
This
uses an outer join so offset-only rows (e.g. the rest of a prior
day when
the current day is still in progress) are preserved.
+ :param x_axis_label: The query's temporal x-axis label, used to pick
the
+ temporal join key when no time grain is set.
"""
join_column_producer =
app.config["TIME_GRAIN_JOIN_COLUMN_PRODUCERS"].get(
time_grain
)
-
- if not time_grain:
- has_temporal_join_key = any(
- pd.api.types.is_datetime64_any_dtype(df[key])
- for key in join_keys
- if key in df.columns
- )
- if has_temporal_join_key:
- has_relative_offset = any(
- not (
- self.is_valid_date_range(offset)
- and feature_flag_manager.is_feature_enabled(
- "DATE_RANGE_TIMESHIFTS_ENABLED"
- )
- )
- for offset in offset_dfs
- )
- if has_relative_offset:
- raise QueryObjectValidationError(
- _("Time Grain must be specified when using Time
Comparison.")
- )
+ original_columns = list(df.columns)
Review Comment:
Not changed: the flagged line is a tuple-unpacking assignment (`offset_df,
actual_join_keys = ...`), which cannot carry an inline annotation in Python;
both types are fully inferred from `_determine_join_keys`'s annotated return
type.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
