fix(dataset): correct Hours Offset filter bounds and grain-truncation order - #43173
Draft
sadpandajoe wants to merge 3 commits into
Draft
fix(dataset): correct Hours Offset filter bounds and grain-truncation order#43173sadpandajoe wants to merge 3 commits into
sadpandajoe wants to merge 3 commits into
Conversation
… order The dataset "Hours offset" setting had two independent bugs: 1. Filter bounds shifted a DATE-typed column's literal bound by the raw offset before rendering it as a date-only value. A sub-24h offset could move which calendar day a bound truncated to, silently admitting an out-of-range day on one end and dropping the last requested day on the other. The offset is now quantized to whole days (truncating toward zero) before it's applied to a DATE column's bounds, so a sub-day offset never moves the selected day window. 2. The offset was applied only in pandas, after the database had already truncated a temporal axis to its time grain using the raw (unshifted) value. A row near a grain boundary could bucket to the wrong day, and the displayed label (computed via a different path) could disagree with the filter bounds. The offset is now applied in SQL, before grain truncation, at the axis-construction call sites -- gated behind a new `apply_dataset_offset` opt-in so filter construction and other unrelated callers are unaffected. A new `sql_shifted_temporal_labels` signal tells dataframe normalization which columns were already shifted in SQL so the legacy pandas-side offset isn't re-applied on top. Since the SQL-side shift depends on dialect-specific interval syntax, it's gated behind a new `BaseEngineSpec.supports_temporal_column_shift` capability flag (default False), enabled explicitly on the two engines it's been verified against (Postgres, SQLite). Engines that don't opt in keep the prior pandas-only behavior for the grain-truncation-order bug -- no new regression -- while gaining the DATE-bound quantization fix unconditionally (that part is engine-agnostic).
Covers both defects across an offset x grain x column-type matrix: - DATE-column filter bounds stay on the requested whole-day window for offsets 0/+1/+24/+25/-1/-25 (the bug admitted/dropped a day for any sub-24h offset); TIMESTAMP columns keep exact-hour bounds as a control. - Grained physical and adhoc BASE_AXIS temporal axes apply the offset exactly once, before grain truncation, matching the exact SQL literal and the resulting bucket for each combination -- including the case that previously applied the offset zero times for an ungrained axis, and negative sub-day DATE offsets that must not move the bucket. Physical and adhoc filter SQL are asserted byte-identical to pre-fix output for the explicitly out-of-scope grained-filter paths. - The new per-engine capability gate correctly excludes every engine that doesn't support the SQL-side shift (falls back to the pre-fix pandas-only offset, verified against the real code path with no shift SQL emitted) and correctly includes Postgres/SQLite (compiled SQL asserted exactly, across signed and >=24h offsets).
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43173 +/- ##
=======================================
Coverage 66.67% 66.68%
=======================================
Files 2871 2871
Lines 163328 163362 +34
Branches 37674 37681 +7
=======================================
+ Hits 108904 108933 +29
+ Misses 52296 52295 -1
- Partials 2128 2134 +6
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:
|
…t param normalize_df now accepts an optional third sql_shifted_temporal_labels parameter (passed positionally from processing_time_offsets). Three pre-existing mocks in this file stubbed normalize_df with a fixed two-argument lambda, which broke when the call site started passing a third positional argument.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
The dataset "Hours offset" setting had two independent bugs:
Filter bounds could admit or drop a day. For a DATE-typed column, the
filter bound was shifted by the raw Hours Offset before being rendered as a
date-only literal. Any sub-24h offset could move which calendar day a bound
truncated to -- silently admitting an out-of-range day on one end while
dropping the last requested day on the other. TIMESTAMP-typed columns were
unaffected (they keep exact-hour precision).
Displayed labels could disagree with the filter bounds. The offset was
applied only in pandas, after the database had already truncated a temporal
axis to its time grain using the raw (unshifted) value. A row near a grain
boundary could bucket to the wrong day, so the label shown to the user could
disagree with which rows the filter actually selected.
FIX
The offset is now quantized to whole days (truncating toward zero) before
being applied to a DATE column's filter bounds, so a sub-day offset never
moves the selected day window. This part is engine-agnostic.
The offset is now applied in SQL, before grain truncation, at the
axis-construction call sites -- gated behind a new
apply_dataset_offsetopt-in parameter so filter construction and other callers are unaffected.
A new
sql_shifted_temporal_labelssignal tells dataframe normalizationwhich columns were already shifted in SQL, so the offset isn't re-applied
on top in pandas.
Since the SQL-side shift depends on dialect-specific interval syntax, it's
gated behind a new
BaseEngineSpec.supports_temporal_column_shiftcapability flag (default
False), enabled explicitly on the two enginesit's verified against (PostgreSQL, SQLite). Engines that don't opt in keep
the prior pandas-only behavior for bug (2) -- no new regression -- while
unconditionally gaining the bound-quantization fix for bug (1), which is
engine-agnostic.
TESTING
Added
tests/unit_tests/models/test_hours_offset_bound_truncation.py, coveringboth bugs across an offset x grain x column-type matrix: DATE-column filter
bounds across offsets 0/+1/+24/+25/-1/-25 (with a TIMESTAMP control), grained
physical and adhoc BASE_AXIS temporal axes applying the offset exactly once
before truncation, negative sub-day DATE offsets that must not move the
bucket, byte-identical SQL for the explicitly out-of-scope grained-filter
paths, and the new per-engine capability gate (including its pandas-fallback
path for unsupported engines).
ADDITIONAL SCOPE
Grained temporal filters combined with a shifted grained axis are out of
scope for this change (the filter path is unchanged) and may need a follow-up.
DST handling and the
extra.timezoneoverride path are also unchanged.