aminghadersohi commented on code in PR #45671:
URL: https://github.com/apache/superset/pull/45671#discussion_r4416305277
##########
superset/common/query_object.py:
##########
@@ -782,11 +784,54 @@ def exec_post_processing(self, df: DataFrame) ->
DataFrame:
func = extra_ops[operation]
options = post_process.get("options", {})
if operation == "resample":
- options = self._resolve_resample_options(options)
+ options = self._resolve_resample_options(options, df)
df = func(df, **options)
return df
- def _resolve_resample_options(self, options: dict[str, Any]) -> dict[str,
Any]:
+ def _resample_display_bound(
+ self, bound: datetime | None, df: DataFrame
+ ) -> datetime | None:
+ """Align explicit instants with a datasource's normalized display
frame."""
+ if (
+ bound is None
+ or bound.utcoffset() is None
+ or self.datasource is None
+ or getattr(df.index, "tz", None) is not None
+ ):
+ return bound
+ column_type = next(
+ (
+ col.type
+ for col in getattr(self.datasource, "columns", [])
+ if col.column_name == self.granularity
Review Comment:
The DATE check reads only `self.granularity`, which is `None` for x-axis
charts: a DATE x-axis with `fill_time_range` over `2024-05-01T00:00:00+08:00 :
2024-06-01T00:00:00+08:00` pads an extra `2024-04-30` row outside the SQL
filter.
##########
tests/unit_tests/models/test_time_bound_consumers.py:
##########
@@ -0,0 +1,308 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Offset-aware quarter bounds through their real downstream consumers."""
+
+import sqlite3
+from datetime import datetime, timedelta, timezone
+
+import pandas as pd
+import pytest
+from flask import Flask
+
+from superset.common.query_object import QueryObject
+from superset.connectors.sqla.models import SqlaTable, TableColumn
+from superset.jinja_context import ExtraCache
+from superset.mcp_service.dashboard_scope import (
+ _rewrite_dataset_request,
+ DashboardConstraints,
+ intersect_time_ranges,
+)
+from superset.models.core import Database
+from superset.utils.core import DateColumn, normalize_dttm_col
+from superset.utils.date_parser import (
+ datetime_eval,
+ get_past_or_future,
+ get_since_until,
+)
+
+QUARTER_RANGE = (
+ "DATETRUNC(DATETIME('2024-05-31T12:00:00+08:00'), QUARTER) : "
+ "DATETIME('2024-07-01T00:00:00+08:00')"
+)
+
+
+def _dataset(
+ extra: str | None = None,
+ offset: int = 0,
+ column_type: str = "TIMESTAMP",
+ uri: str = "sqlite://",
+) -> tuple[SqlaTable, TableColumn]:
+ """Build a real datasource with temporal metadata without connecting to a
DB."""
+ col = TableColumn(column_name="ts", type=column_type, is_dttm=True)
+ table = SqlaTable(
+ table_name="events",
+ columns=[col],
+ main_dttm_col="ts",
+ database=Database(database_name="test", sqlalchemy_uri=uri),
+ extra=extra,
+ offset=offset,
+ )
+ return table, col
+
+
[email protected]("unit", ["", ", DAY"])
+def test_review_datediff_mixed_quarter_bounds(unit: str) -> None:
+ """DATEDIFF and nesting it in DATEADD retain the legacy mixed-bound
duration."""
+ diff = (
+ "DATEDIFF(DATETRUNC(DATETIME('2024-05-31T12:00:00+08:00'), QUARTER), "
+ f"DATETIME('2024-06-01'){unit})"
+ )
+ assert datetime_eval(diff) == 61
+ assert datetime_eval(f"DATEADD(DATETIME('2024-01-01'), {diff}, DAY)") ==
datetime(
+ 2024, 3, 2
+ )
+
+
[email protected]("phrase", ["1 year ago", "1 month ago", "1 quarter
ago"])
+def test_review_comparison_shift_keeps_offset(app: Flask, phrase: str) -> None:
+ """Comparison bounds keep their input offset through the real shift and
SQL."""
+ table, col = _dataset('{"timezone": "Europe/Berlin"}')
+ start, end = get_since_until(QUARTER_RANGE)
+ assert start is not None
+ assert end is not None
+ shifted_start = get_past_or_future(phrase, start)
+ shifted_end = get_past_or_future(phrase, end)
+ assert shifted_start.utcoffset() == timedelta(hours=8)
+ assert shifted_end.utcoffset() == timedelta(hours=8)
+ with app.app_context():
+ sql = str(table.get_time_filter(col, shifted_start, shifted_end))
+ assert shifted_start.astimezone(timezone.utc).strftime("%Y-%m-%d
%H:%M:%S") in sql
+
+
[email protected]("uri", ["sqlite://", "postgresql://"])
+def test_review_date_columns_preserve_calendar_dates(app: Flask, uri: str) ->
None:
+ """DATE literals retain the input calendar date rather than the prior UTC
date."""
+ table, col = _dataset(column_type="DATE", uri=uri)
+ start, end = get_since_until(
+ "2024-05-01T00:00:00+08:00 : 2024-06-01T00:00:00+08:00"
+ )
+ with app.app_context():
+ adjusted = table.adjust_time_bounds(start, end, col)
+ sql = str(table.get_time_filter(col, start, end))
+ assert adjusted == (datetime(2024, 5, 1), datetime(2024, 6, 1))
+ assert "2024-05-01" in sql
+ assert "2024-06-01" in sql
+ assert "2024-04-30" not in sql
+
+
[email protected]("extra", [None, '{"timezone": "Invalid/Zone"}'])
+def test_review_hour_offset_applies_only_to_naive_bounds(
+ app: Flask, extra: str | None
+) -> None:
+ """An aware start is converted once; the naive end uses the dataset Hour
Offset."""
+ table, col = _dataset(extra, offset=8)
+ start, end = get_since_until(
+ "DATETRUNC(DATETIME('2024-05-31T12:00:00+08:00'), QUARTER) :
2024-07-01"
+ )
+ with app.app_context():
+ assert table.adjust_time_bounds(start, end, col) == (
+ datetime(2024, 3, 31, 16),
+ datetime(2024, 6, 30, 16),
+ )
+
+
[email protected]("extra", [None, '{"timezone": "Europe/Berlin"}'])
+def test_review_jinja_filter_matches_chart_and_sqlite_rows(
+ app: Flask, extra: str | None
+) -> None:
+ """The Jinja macro and chart clause select the same actual SQLite rows."""
+ table, col = _dataset(extra, offset=8)
+ start, end = get_since_until(QUARTER_RANGE)
+ with app.test_request_context(json={"queries": [{"time_range":
QUARTER_RANGE}]}):
+ macro = ExtraCache(database=table.database,
table=table).get_time_filter(
+ "ts", target_type="TIMESTAMP"
+ )
+ chart = table.get_time_filter(col, start, end)
+ with sqlite3.connect(":memory:") as connection:
+ connection.execute("CREATE TABLE events (ts TEXT)")
+ connection.executemany(
+ "INSERT INTO events VALUES (?)",
+ [
+ ("2024-03-31 15:59:59",),
+ ("2024-03-31 20:00:00",),
+ ("2024-06-30 16:00:00",),
+ ],
+ )
+ macro_rows = connection.execute(
+ f"SELECT ts FROM events WHERE ts >= {macro.from_expr} " # noqa:
S608
+ f"AND ts < {macro.to_expr}"
+ ).fetchall()
+ chart_rows = connection.execute(
+ f"SELECT ts FROM events WHERE {chart}" # noqa: S608
+ ).fetchall()
+ assert macro_rows == chart_rows == [("2024-03-31 20:00:00",)]
+
+
[email protected]("index_tz", [None, "UTC", "Europe/Berlin"])
+def test_review_fill_time_range_matches_queried_instants(index_tz: str | None)
-> None:
+ """Hourly fill covers the UTC query period, including its exclusive end."""
+ start, end = get_since_until(QUARTER_RANGE)
+ query = QueryObject(
+ row_limit=1,
+ from_dttm=start,
+ to_dttm=end,
+ post_processing=[
+ {
+ "operation": "resample",
+ "options": {
+ "rule": "1h",
+ "method": "asfreq",
+ "fill_value": 0,
+ "fill_time_range": True,
+ },
+ }
+ ],
+ )
+ frame = pd.DataFrame(
+ {"value": [1.0]}, index=pd.DatetimeIndex(["2024-05-01"], tz=index_tz)
+ )
+ result = query.exec_post_processing(frame)
+ index = result.index if index_tz is None else result.index.tz_convert(None)
+ assert index[0] == pd.Timestamp("2024-03-31 16:00:00")
+ assert index[-1] == pd.Timestamp("2024-06-30 15:00:00")
+ assert len(index) == 2184
+
+
[email protected](
+ "extra,offset,display_start,display_end",
+ [
+ ('{"timezone": "Europe/Berlin"}', 0, "2024-03-31 18:00", "2024-06-30
17:00"),
+ (None, 8, "2024-04-01 00:00", "2024-06-30 23:00"),
+ ],
+)
+def test_review_fill_time_range_uses_normalized_display_frame(
+ extra: str | None, offset: int, display_start: str, display_end: str
+) -> None:
+ """Padding uses the same display frame as the datasource-normalized
data."""
+ table, _ = _dataset(extra, offset)
+ start, end = get_since_until(QUARTER_RANGE)
+ query = QueryObject(
+ datasource=table,
+ row_limit=1,
+ from_dttm=start,
+ to_dttm=end,
+ post_processing=[
+ {
+ "operation": "resample",
+ "options": {
+ "rule": "1h",
+ "method": "asfreq",
+ "fill_value": 0,
+ "fill_time_range": True,
+ },
+ }
+ ],
+ )
+ frame = pd.DataFrame({"ts": pd.to_datetime(["2024-05-01"]), "value":
[1.0]})
+ normalize_dttm_col(
+ frame,
+ (
+ DateColumn(
+ col_label="ts", offset=offset,
timezone=table.get_dataset_timezone()
+ ),
+ ),
+ )
+ result = query.exec_post_processing(frame.set_index("ts"))
+ assert result.index[0] == pd.Timestamp(display_start)
+ assert result.index[-1] == pd.Timestamp(display_end)
+
+
+def test_review_mcp_mixed_result_survives_reparse() -> None:
+ """A valid instant range stays valid when the selected endpoints are
mixed."""
+ table, col = _dataset()
+ result = intersect_time_ranges(
+ "2024-04-01T00:00:00+08:00 : 2024-04-02T00:00:00+08:00",
+ "2024-03-31T20:00:00 : 2024-04-03",
+ )
+ assert table.adjust_time_bounds(*get_since_until(result), col) == (
+ datetime(2024, 3, 31, 20),
+ datetime(2024, 4, 1, 16),
+ )
+
+
+def test_review_fill_time_range_retains_date_column_calendar() -> None:
+ """DATE padding retains the same calendar boundaries as the SQL filter."""
+ table, _ = _dataset(column_type="DATE")
+ start, end = get_since_until(
+ "2024-05-01T00:00:00+08:00 : 2024-06-01T00:00:00+08:00"
+ )
+ query = QueryObject(
+ datasource=table,
+ granularity="ts",
+ row_limit=1,
+ from_dttm=start,
+ to_dttm=end,
+ post_processing=[
+ {
+ "operation": "resample",
+ "options": {
+ "rule": "1h",
+ "method": "asfreq",
+ "fill_value": 0,
+ "fill_time_range": True,
+ },
+ }
+ ],
+ )
+ frame = pd.DataFrame({"value": [1.0]},
index=pd.to_datetime(["2024-05-15"]))
+ result = query.exec_post_processing(frame)
+ assert result.index[0] == pd.Timestamp("2024-05-01")
+ assert result.index[-1] == pd.Timestamp("2024-05-31 23:00:00")
+
+
+def test_review_mcp_intersection_cannot_widen_offset_scope() -> None:
+ """03:00 UTC cannot replace a dashboard scope beginning at 05:00 UTC."""
+ result = intersect_time_ranges(
+ "DATETRUNC(DATETIME('2024-05-31T12:00:00-05:00'), QUARTER) :
2024-07-01",
+ "2024-04-01T03:00:00 : 2024-06-01",
+ )
+ table, col = _dataset()
+ bounds = get_since_until(result)
+ assert table.adjust_time_bounds(*bounds, col)[0] == datetime(2024, 4, 1, 5)
+
+
[email protected](
+ "extra,offset", [(None, 8), ('{"timezone": "Europe/Berlin"}', 0)]
+)
+def test_review_mcp_rewrite_uses_dataset_bound_semantics(
+ extra: str | None, offset: int
+) -> None:
+ """The dataset request rewrite intersects using the same adjustments as
SQL."""
+ table, col = _dataset(extra, offset)
+ scope_range = "2024-04-01T00:00:00-05:00 : 2024-07-01"
Review Comment:
Nit: This passes with `normalize_bound=normalize_bound` dropped from
`_rewrite_dataset_request`: the scope start wins under either rule here. An
`America/Los_Angeles` dataset (model `03:00` is `10:00Z`) would pin it.
##########
superset/models/helpers.py:
##########
@@ -5881,48 +5894,51 @@ def adjust_time_bounds(
against the engine -- see exactly the instants the clause compares
against, rather than a copy of this logic that can drift.
"""
- dataset_timezone = self.get_dataset_timezone()
-
- if dataset_timezone and (start_dttm or end_dttm):
+ tz = None
+ if dataset_timezone := self.get_dataset_timezone():
try:
tz = ZoneInfo(dataset_timezone)
-
- # The datetimes from the UI are naive (no timezone info). We
- # interpret them as being in the dataset's configured timezone
and
- # convert them to UTC for comparison with UTC-stored data.
- if start_dttm is not None:
- start_dttm = (
- start_dttm.replace(tzinfo=tz)
- .astimezone(timezone.utc)
- .replace(tzinfo=None)
- )
- if end_dttm is not None:
- end_dttm = (
- end_dttm.replace(tzinfo=tz)
- .astimezone(timezone.utc)
- .replace(tzinfo=None)
- )
except (ZoneInfoNotFoundError, TypeError):
logger.warning(
"Invalid timezone %r in dataset extra; falling back to
Hour "
"Offset if configured",
dataset_timezone,
)
- dataset_timezone = None
-
- if not dataset_timezone and (offset_hours := getattr(self, "offset",
0) or 0):
+ column_type = target_type or (time_col.type if time_col is not None
else None)
+ sqla_type = (
+ self.db_engine_spec.get_sqla_column_type(column_type)
+ if column_type
+ else None
+ )
+ is_date = isinstance(sqla_type, sa.Date) and not isinstance(
Review Comment:
Date-only string columns still lose a day: a VARCHAR column with
`python_date_format` `%Y-%m-%d` and range `2024-04-01T00:00:00+08:00 :
2024-07-01T00:00:00+08:00` renders Postgres `ts >= '2024-03-31' AND ts <
'2024-06-30'`; master kept `2024-04-01`/`2024-07-01`.
##########
superset/jinja_context.py:
##########
@@ -727,6 +728,37 @@ def _get_filters_from_query_context(
return filters
# pylint: disable=too-many-arguments
+ def _normalize_time_filter_bounds(
+ self,
+ start: datetime | None,
+ end: datetime | None,
+ column: str | None,
+ target_type: str | None,
+ ) -> tuple[datetime | None, datetime | None]:
+ """Use the same bound interpretation as the datasource's chart
filter."""
+ if self.table:
+ time_col = next(
+ (
+ col
+ for col in self.table.columns
+ if col.column_name == (column or self.table.main_dttm_col)
+ ),
+ None,
+ )
+ return self.table.adjust_time_bounds(
+ start, end, time_col, target_type=target_type
+ )
Review Comment:
This also shifts naive ranges: with dataset timezone `Asia/Shanghai` or Hour
Offset 8, `get_time_filter('ds', strftime='%Y-%m-%d')` over `2024-05-01 :
2024-06-01` returns `2024-04-30`/`2024-05-31` where master returned
`2024-05-01`/`2024-06-01`.
```suggestion
adjusted = self.table.adjust_time_bounds(
start, end, time_col, target_type=target_type
)
# Naive bounds keep the macro's unshifted wall-clock values.
return (
adjusted[0] if start and start.utcoffset() is not None else
start,
adjusted[1] if end and end.utcoffset() is not None else end,
)
```
--
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]
