sadpandajoe opened a new pull request, #43315:
URL: https://github.com/apache/superset/pull/43315
### SUMMARY
A post-merge review of #42054 (grain-less time-comparison alignment) found
three concrete correctness bugs in the new alignment path in
`superset/models/helpers.py` and `superset/utils/date_parser.py`. This PR fixes
all three:
1. **DST-fold row duplication**: the synthetic wall-clock join key used for
grain-less alignment discarded UTC-offset/fold information, so a repeated local
hour (the DST "fall back" transition) could collapse two distinct historical
instants onto one join key. A left join then fanned a single main-series row
out into duplicates, inflating totals. This now raises an actionable validation
error identifying the ambiguous alignment instead of silently duplicating rows.
Duplicates already present in the raw (pre-normalization) data, and duplicates
from ordinary dimension multiplicity or month-end date clamping, are correctly
left alone.
2. **Weekday/month-name offset anchors misclassified as constant deltas**:
`is_constant_human_timedelta()` probed only two same-date timestamps, so
date-dependent anchors like `friday` or `june` passed as if they were constant
deltas (like `1 month ago`), producing misaligned or duplicated comparison
rows. A third, differently-dated probe now correctly rejects these anchors
while leaving genuine relative deltas (`1 month ago`, `52 weeks ago`, `1 year
ago`, ...) unaffected.
3. **Custom SQL / string-backed temporal axes silently bypassing
alignment**: the alignment path required the temporal axis to already be
`datetime64` dtype, so a Custom SQL temporal expression resolving to a
string/object-dtype column (e.g. on SQLite) silently fell back to an unaligned
raw-key join — in the worst case, comparing a period against itself. Axes that
are direct references to a physical dataset column with authoritative temporal
metadata (`is_dttm`/`type_generic`) are now losslessly coerced and aligned; a
value that fails to parse raises an actionable error instead of silently
misaligning.
### Scope and known follow-ups
- Item 3 is scoped to **direct references to a physical dataset column**
with declared temporal metadata, and to a **single offset**. Fully
computed/derived Custom SQL temporal expressions (e.g. `DATE_TRUNC(...)`) are
not covered by this PR and keep today's behavior; adding coverage would mean
either a new `AdhocColumn` metadata field or forcing a per-request DB type
probe, both real design decisions out of scope here.
- Two related, independently-reachable correctness gaps found in the same
review are **explicitly out of scope** for this PR and are recommended as
follow-ups:
- `superset/semantic_layers/mapper.py`'s time-comparison path merges on
raw (unaligned) temporal keys. This is a separate pipeline (not touched by
#42054) gated behind the `SEMANTIC_LAYERS` feature flag, which defaults off.
- `processing_time_offsets()` derives each additional offset's temporal
filter from the previous offset's already-shifted filter rather than the
original query, on the branch used by a non-datetime x-axis. This is
pre-existing (predates #42054) and means a **single** offset on a
string/Custom-SQL axis is now correctly aligned by this PR, but **multiple**
offsets on such an axis are not yet.
- Two narrow, non-blocking edge cases remain from independent review and are
good starter follow-ups: the DST-vs-generic validation message can be imprecise
when unrelated duplicate groups coexist in the same query (the query still
correctly fails either way); and the out-of-range-date retry path uses strict
parsing for a declared-but-non-exact datetime format, which can raise on a
combination that the primary parse path would otherwise tolerate (fails loudly,
not silently-wrong).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — this is a backend query-result correctness fix with no
UI/rendering change. Behavior is demonstrated by the unit and integration tests
below.
### TESTING INSTRUCTIONS
- `pytest tests/unit_tests/common/test_time_shifts.py
tests/unit_tests/utils/date_parser_tests.py`
- `pytest
tests/integration_tests/query_context_tests.py::test_time_offset_without_grain_aligns_direct_custom_sql_temporal_axis`
(requires the SQLite integration test database)
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
