Repository navigation
fix(ag-grid): honor dataset hour offset in time range filters - #41391
Conversation
Result timestamps are displayed shifted by +offset hours (normalize_df / DateColumn), but get_time_filter compared the raw stored values against the requested range. With a non-zero dataset Hour Offset this made a date selection land on the wrong calendar day (e.g. selecting Mar 24 returned Mar 23 rows). Shift the time-filter bounds by -offset hours so the filter is evaluated in the same offset-adjusted space that is displayed. Covers the dashboard/explore time-range filter path; the AG Grid column-level date filter builds its own raw SQL and is tracked separately. Add unit tests.
amaannawab923
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41391 +/- ##
==========================================
- Coverage 64.79% 64.79% -0.01%
==========================================
Files 2692 2692
Lines 149038 149043 +5
Branches 34398 34401 +3
==========================================
+ Hits 96573 96574 +1
- Misses 50696 50698 +2
- Partials 1769 1771 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
EnxDev's Review Agent — #41391 · HEAD ebcc990comment — 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: 🟡 Should-fix
🔵 Nits
🙌 Praise
|
dosubot
Bot
added
data:dataset
Code Review Agent Run #52b4ddActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The local variable offset_hours: int = getattr(self, "offset", 0) or 0There are no other comments on this PR to address. Would you like me to check for any other potential type hint improvements in the modified file? superset/models/helpers.py |
The #104810 fix makes get_time_filter honor the dataset Hour Offset, shifting temporal filter bounds. Two pre-existing integration tests assert raw date literals against the shared birth_names table, which other tests in the suite can leave with a non-zero offset; the shift then broke their expected SQL. Pin the dataset offset to 0 in test_where_operators (which tests filter operators, not offset) and in test_time_offset_comparison_queries_use_chart_row_limit (which asserts comparison-window date literals), keeping both deterministic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
87c851b to
76842c7
Compare
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
From manual testing:
- Before
- After
Pre-commit auto-fixes on the temporal-filter offset code: use the walrus operator for the offset guard in get_time_filter and collapse a one-line test helper signature. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Bito Automatic Review Skipped – PR Already Merged |
Rebase resolution for the dataset-timezone time filter against master's dataset "Hour Offset" support (apache#41391). The timezone conversion takes precedence and the hour-offset shift is kept as the fallback, matching the precedence already used in normalize_dttm_col. Extracted the boundary adjustment into _adjust_time_filter_boundaries so get_time_filter stays under the mccabe complexity limit. Co-Authored-By: bot_apk <apk@cognition.ai>
…xtra values Rebases the DST-aware timezone conversion onto master, which since merged apache#41391's fixed Hour Offset shift for the same get_time_filter method. The two mechanisms now layer instead of conflict: a configured extra.timezone takes precedence (it is strictly more correct, since it handles DST), and the legacy Hour Offset still applies exactly as before when no timezone is configured. Also addresses two outstanding review threads on get_time_filter: - get_dataset_timezone now validates that extra["timezone"] is a string before returning it, since extra is arbitrary user-supplied JSON and a non-string value would otherwise reach ZoneInfo() and raise TypeError. get_time_filter also catches TypeError alongside ZoneInfoNotFoundError as defense in depth. - Confirmed the epoch-rendering concern (naive datetime .timestamp() being interpreted in the server's local timezone) is already fixed on master via apache#39782's UTC-anchoring in dttm_sql_literal; added a regression test tying it to the dataset-timezone path specifically. Adds test coverage for the reconciliation (offset-only fallback, timezone taking precedence over a configured offset) and the non-string timezone guard. Co-Authored-By: Claude <noreply@anthropic.com>
3 participants
SUMMARY
When a dataset is configured with a non-zero Hour Offset, date filters were landing on the wrong calendar day — e.g. filtering for a given date could return the previous day's rows.
The root cause is an asymmetry: result timestamps are shifted by
+offsethours for display (vianormalize_df/DateColumn), but the time-range filter was still comparing against the raw stored values. So the data you saw was offset-adjusted while the filter wasn't, and the two disagreed by the offset.The fix shifts the time-filter bounds by
-offsethours inget_time_filter, so the filter is evaluated in the same offset-adjusted space that's displayed. This covers the dashboard/explore time-range filter path. (The AG Grid column-level date filter builds its own SQL and isn't part of this change — I've left a note to follow that up separately.)TESTING INSTRUCTIONS
-4).Unit tests added (no offset → raw bounds; positive offset → bounds shifted back; negative offset → bounds shifted forward). Existing time-filter tests still pass.
ADDITIONAL INFORMATION
This affects any dataset configured with a non-zero hour offset.