bito-code-review[bot] commented on code in PR #43173: URL: https://github.com/apache/superset/pull/43173#discussion_r3825503351
########## tests/unit_tests/models/test_hours_offset_bound_truncation.py: ########## @@ -0,0 +1,702 @@ +# 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. +"""Regression guards for dataset "Hours offset" bound and grain handling. + +Two independent defects are covered here. + +Defect 1 -- DATE-column filter bounds use a whole-day effective offset so rendering + date-only literals cannot discard a sub-day remainder and move the window. + +Defect 2 -- grained axis expressions apply the dataset offset in SQL before time + grain truncation. Dataframe normalization suppresses its legacy post-query + offset only for labels that were shifted in SQL. +""" + +from __future__ import annotations + +from contextlib import contextmanager +from datetime import date, datetime + +import pandas as pd +import pytest +from flask import Flask +from pytest_mock import MockerFixture +from sqlalchemy import column, create_engine, DateTime +from sqlalchemy.dialects import postgresql, sqlite +from sqlalchemy.engine import Engine +from sqlalchemy.orm.session import Session +from sqlalchemy.pool import StaticPool + +from superset.common.query_object import QueryObject +from superset.connectors.sqla.models import SqlaTable, TableColumn +from superset.db_engine_specs.base import BaseEngineSpec +from superset.db_engine_specs.postgres import PostgresEngineSpec +from superset.db_engine_specs.sqlite import SqliteEngineSpec +from superset.models.core import Database +from superset.superset_typing import AdhocColumn, QueryObjectDict + +# --------------------------------------------------------------------------- +# Defect 1 -- DATE-column filter bound literal truncated to day precision +# --------------------------------------------------------------------------- + + +def _pg_dataset(offset: int, col_type: str) -> SqlaTable: + """A Postgres-backed dataset with a single temporal column of ``col_type`` + and the given dataset Hours ``offset``.""" + database = Database( + id=1, + database_name="pg", + # A postgres:// URI selects PostgresEngineSpec; the SQL is only compiled, + # never executed, so no live server is required. + sqlalchemy_uri="postgresql://u:p@localhost:5432/db", + ) + columns = [ + TableColumn(column_name="loan_date", is_dttm=1, type=col_type), + TableColumn(column_name="value", type="INTEGER"), + ] + return SqlaTable( + table_name="loans", + columns=columns, + main_dttm_col="loan_date", + database=database, + offset=offset, + ) + + +def _generated_sql(dataset: SqlaTable, mocker: MockerFixture, app: Flask) -> str: + mocker.patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[], + ) + mocker.patch( + "superset.connectors.sqla.models.security_manager.is_guest_user", + return_value=False, + ) + # Requested window: the whole month of August 2026, i.e. [2026-08-01, 2026-09-01). + query_obj: QueryObjectDict = { + "granularity": "loan_date", + "from_dttm": datetime(2026, 8, 1), + "to_dttm": datetime(2026, 9, 1), + "is_timeseries": False, + "filter": [ + { + "col": "loan_date", + "op": "TEMPORAL_RANGE", + "val": "2026-08-01 : 2026-09-01", + } + ], + "metrics": [], + "columns": ["value"], + } + with app.test_request_context(): + return dataset.get_query_str_extended(query_obj, mutate=False).sql + + +def test_date_column_hours_offset_does_not_shift_selected_day_window( + mocker: MockerFixture, app: Flask +) -> None: + """A pure ``DATE`` column stores calendar dates at midnight, so a +1h Hours + offset can never move a value across a day boundary: the selected window must + stay 2026-08-01 .. 2026-08-31 (identical to offset 0). + + The bug shifts the bounds back 1h (2026-07-31 23:00 / 2026-08-31 23:00) and + then truncates each with ``.date()`` -> ``TO_DATE('2026-07-31')`` / + ``TO_DATE('2026-08-31')``. That window, [2026-07-31, 2026-08-31), admits the + out-of-range day 2026-07-31 and silently drops the last requested day, + 2026-08-31. + """ + sql = _generated_sql(_pg_dataset(1, "DATE"), mocker, app) + + assert ">= TO_DATE('2026-08-01'" in sql, sql + assert "< TO_DATE('2026-09-01'" in sql, sql + # The lower bound must not admit the day before the requested range. + assert ">= TO_DATE('2026-07-31'" not in sql, ( + f"DATE-column +1h offset admits out-of-range day 2026-07-31; SQL was:\n{sql}" + ) + # The upper bound must not drop the last requested day (2026-08-31). + assert "< TO_DATE('2026-08-31'" not in sql, ( + f"DATE-column +1h offset drops last requested day 2026-08-31; SQL was:\n{sql}" + ) + + [email protected]( + ("offset", "expected_start", "expected_end"), + [ + (0, "2026-08-01", "2026-09-01"), + (1, "2026-08-01", "2026-09-01"), + (24, "2026-07-31", "2026-08-31"), + (25, "2026-07-31", "2026-08-31"), + (-1, "2026-08-01", "2026-09-01"), + (-25, "2026-08-02", "2026-09-02"), + ], +) +def test_date_column_hours_offset_uses_whole_day_bounds( + offset: int, + expected_start: str, + expected_end: str, + mocker: MockerFixture, + app: Flask, +) -> None: + """DATE bounds discard sub-day remainders symmetrically around zero.""" + sql = _generated_sql(_pg_dataset(offset, "DATE"), mocker, app) + + assert f">= TO_DATE('{expected_start}'" in sql, sql + assert f"< TO_DATE('{expected_end}'" in sql, sql + + +def test_timestamp_column_hours_offset_preserves_exact_hour_bounds( + mocker: MockerFixture, app: Flask +) -> None: + """Control for Defect 1: the same +1h offset on a ``TIMESTAMP`` column keeps + exact-hour precision (2026-07-31 23:00:00 / 2026-08-31 23:00:00) and loses + nothing. This passes today and documents that the defect is DATE-specific.""" + sql = _generated_sql(_pg_dataset(1, "TIMESTAMP"), mocker, app) + + assert "2026-07-31 23:00:00" in sql, sql + assert "2026-08-31 23:00:00" in sql, sql + + [email protected]( + ("offset", "expected_start", "expected_end"), + [ + (0, "2026-08-01 00:00:00", "2026-09-01 00:00:00"), + (1, "2026-07-31 23:00:00", "2026-08-31 23:00:00"), + (24, "2026-07-31 00:00:00", "2026-08-31 00:00:00"), + (25, "2026-07-30 23:00:00", "2026-08-30 23:00:00"), + (-1, "2026-08-01 01:00:00", "2026-09-01 01:00:00"), + (-25, "2026-08-02 01:00:00", "2026-09-02 01:00:00"), + ], +) +def test_timestamp_column_hours_offset_uses_exact_hour_bounds( + offset: int, + expected_start: str, + expected_end: str, + mocker: MockerFixture, + app: Flask, +) -> None: + """Timestamp bounds preserve every configured offset hour.""" + sql = _generated_sql(_pg_dataset(offset, "TIMESTAMP"), mocker, app) + + assert expected_start in sql, sql + assert expected_end in sql, sql + + +def test_datetime_named_column_keeps_exact_hour_bounds( + mocker: MockerFixture, app: Flask +) -> None: + """A DATETIME type name must not be mistaken for a pure DATE type.""" + sql = _generated_sql(_pg_dataset(1, "DATETIME"), mocker, app) + + assert "2026-07-31 23:00:00" in sql, sql + assert "2026-08-31 23:00:00" in sql, sql + + +# --------------------------------------------------------------------------- +# Defect 2 -- Hours offset applied after DB-side time-grain truncation +# --------------------------------------------------------------------------- + + +def _sqlite_dataset( + mocker: MockerFixture, + offset: int, + column_type: str, + rows: list[str], +) -> tuple[SqlaTable, Engine]: + """Build an executable SQLite dataset with controlled temporal rows.""" + engine = create_engine( + "sqlite://", + connect_args={"check_same_thread": False}, + poolclass=StaticPool, + future=True, + ) + database = Database(database_name="db", sqlalchemy_uri="sqlite://") + connection = engine.raw_connection() + connection.execute(f"CREATE TABLE events (ts {column_type}, val INTEGER)") + connection.executemany( + "INSERT INTO events VALUES (?, 1)", + [(row,) for row in rows], + ) + connection.commit() + + @contextmanager + def mock_get_sqla_engine(catalog=None, schema=None, **kwargs): + yield engine + + mocker.patch.object(database, "get_sqla_engine", new=mock_get_sqla_engine) + mocker.patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[], + ) + mocker.patch( + "superset.connectors.sqla.models.security_manager.is_guest_user", + return_value=False, + ) + + return ( + SqlaTable( + database=database, + schema=None, + table_name="events", + main_dttm_col="ts", + offset=offset, + columns=[ + TableColumn(column_name="ts", is_dttm=True, type=column_type), + TableColumn(column_name="val", type="INTEGER"), + ], + ), + engine, + ) + + +def _physical_axis_query(table: SqlaTable, time_grain: str | None) -> QueryObject: + """Build the physical-axis query shape used by legacy time-series charts.""" + return QueryObject( + datasource=table, + metrics=[{"expressionType": "SQL", "sqlExpression": "COUNT(*)", "label": "ct"}], + columns=[], + granularity="ts", + from_dttm=pd.Timestamp("2026-07-01"), + to_dttm=pd.Timestamp("2026-10-01"), + is_timeseries=True, + extras={"time_grain_sqla": time_grain} if time_grain else {}, + row_limit=100, + ) + + +_EXPECTED_PHYSICAL_AXIS_TIMESTAMPS = { + ("TIMESTAMP", None): { + 0: "2026-08-01 23:30:00", + 1: "2026-08-02 00:30:00", + 24: "2026-08-02 23:30:00", + 25: "2026-08-03 00:30:00", + -1: "2026-08-01 22:30:00", + -25: "2026-07-31 22:30:00", + }, + ("TIMESTAMP", "P1D"): { + 0: "2026-08-01 00:00:00", + 1: "2026-08-02 00:00:00", + 24: "2026-08-02 00:00:00", + 25: "2026-08-03 00:00:00", + -1: "2026-08-01 00:00:00", + -25: "2026-07-31 00:00:00", + }, + ("DATE", None): { + 0: "2026-08-02 00:00:00", + 1: "2026-08-02 01:00:00", + 24: "2026-08-03 00:00:00", + 25: "2026-08-03 01:00:00", + -1: "2026-08-01 23:00:00", + -25: "2026-07-31 23:00:00", + }, + ("DATE", "P1D"): { + 0: "2026-08-02 00:00:00", + 1: "2026-08-02 00:00:00", + 24: "2026-08-03 00:00:00", + 25: "2026-08-03 00:00:00", + -1: "2026-08-02 00:00:00", + -25: "2026-08-01 00:00:00", + }, +} + + [email protected]("column_type", ["DATE", "TIMESTAMP"]) [email protected]("time_grain", [None, "P1D"]) [email protected]("offset", [0, 1, 24, 25, -1, -25]) +def test_physical_axis_offset_matrix( + column_type: str, + time_grain: str | None, + offset: int, + mocker: MockerFixture, +) -> None: + """Physical axes apply each offset once at the precision of their grain.""" + raw_value = "2026-08-02" if column_type == "DATE" else "2026-08-01 23:30:00" + table, _engine = _sqlite_dataset(mocker, offset, column_type, [raw_value]) + + result = table.get_query_result(_physical_axis_query(table, time_grain)) + + assert result.df["__timestamp"].tolist() == [ + pd.Timestamp( + _EXPECTED_PHYSICAL_AXIS_TIMESTAMPS[(column_type, time_grain)][offset] + ) + ] + expected_shifted_labels = {"__timestamp"} if time_grain and offset else set() + assert result.sql_shifted_temporal_labels == expected_shifted_labels + + +def test_ungrained_physical_axis_offset_is_applied_exactly_once( + mocker: MockerFixture, +) -> None: + """An ungrained axis stays on the established pandas-only offset path.""" + table, _engine = _sqlite_dataset( + mocker, + offset=1, + column_type="TIMESTAMP", + rows=["2026-08-01 23:30:00"], + ) + + result = table.get_query_result(_physical_axis_query(table, time_grain=None)) + + assert result.df["__timestamp"].tolist() == [pd.Timestamp("2026-08-02 00:30:00")] + assert result.sql_shifted_temporal_labels == set() + + +def test_negative_subday_date_offset_does_not_move_grained_bucket( + mocker: MockerFixture, +) -> None: + """A negative sub-day offset on a DATE grain quantizes to zero days.""" + table, _engine = _sqlite_dataset( + mocker, + offset=-1, + column_type="DATE", + rows=["2026-08-02"], + ) + + result = table.get_query_result(_physical_axis_query(table, time_grain="P1D")) + + assert result.df["__timestamp"].tolist() == [pd.Timestamp("2026-08-02 00:00:00")] + assert result.sql_shifted_temporal_labels == {"__timestamp"} + assert "+0 hours" not in result.query + + +def test_adhoc_base_axis_offset_is_applied_exactly_once( + mocker: MockerFixture, +) -> None: + """A non-timeseries BASE_AXIS query shifts before its embedded grain.""" + table, _engine = _sqlite_dataset( + mocker, + offset=1, + column_type="TIMESTAMP", + rows=["2026-08-01 23:30:00", "2026-08-02 10:00:00"], + ) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Consolidate duplicate test setup code</b></div> <div id="fix"> Found duplicated test code (21 lines) in tests/unit_tests/models/test_hours_offset_bound_truncation.py at lines 384-404 and 419-439. The duplicate includes AdhocColumn setup, QueryObject creation, and get_query_result assertions. Consider extracting this into a test fixture or helper function to reduce duplication. </div> </div> <small><i>Code Review Run #6456e6</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
