EnxDev commented on PR #41391: URL: https://github.com/apache/superset/pull/41391#issuecomment-4834363737
## EnxDev's Review Agent — apache/superset#41391 · HEAD ebcc990 comment — The fix is correct and well-targeted with good tests, but it's still a Draft and CI is red on auto-fixable formatting. This is a fix PR. I verified the core logic: `normalize_df` shifts displayed timestamps by `+offset` (`superset/utils/core.py:2041`, `df[...] += timedelta(hours=_col.offset)`), while the time filter compares against raw stored values. So shifting the filter bounds by `-offset` is the right direction, and the math matches the tests (offset `+4` → start `2024-01-01 00:00` becomes `2023-12-31 20:00`). `offset` resolves on both concrete subclasses (`SqlaTable` column default `0`, `Query.offset` returns `0`), so datasets without an offset are unaffected — no regression for the default path. ### 🟡 Should-fix - **CI red — pre-commit.** `auto-walrus` and `ruff-format` both rewrote `helpers.py` and the new test file (`2 files reformatted`). Run `pre-commit run --all-files` and commit the auto-fixes — `auto-walrus` will likely collapse `offset_hours = getattr(...)` + `if offset_hours:` into a walrus. mypy/ruff already pass. - **CI red — Python-Integration** (`test-postgres`, `test-mysql`, `test-sqlite`, `test-postgres-required`). Since `offset` defaults to `0` everywhere, this change shouldn't alter existing-test behavior, so these are most likely infra/flake — but confirm they go green on a re-run rather than assuming. - **Title scope is misleading.** `fix(ag-grid): …` — the fix lives in the generic SQLA time-range filter (`get_time_filter`) and, by the author's own note, explicitly does *not* touch the AG Grid column-level date filter. Retitle to `fix(sqla)` / `fix(explore)` so the scope is accurate. ### 🔵 Nits - `superset/models/helpers.py:2807` — `getattr(self, "offset", 0)` default `0` is dead: every concrete subclass defines `offset` (the `ExploreMixin` base raises `NotImplementedError`, which `getattr` would *not* swallow anyway). `self.offset or 0` conveys the intent. Harmless either way. ### 🙌 Praise - `tests/unit_tests/models/test_time_filter_hour_offset.py` — covers zero / positive / negative offset and asserts the exact shifted boundary in the generated SQL (and that the unshifted bound is absent). Real regression guard, not a smoke test. <!-- enxdev-review-agent:ebcc990 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD ebcc990._ -- 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]
