Mark-Onopchenko opened a new pull request, #45797:
URL: https://github.com/apache/superset/pull/45797
### SUMMARY
Fixes #45796.
The **Override time range** and **Override time grain** checkboxes on a
chart-sourced annotation layer (Event, Interval, Time Series) had no effect on
charts with a generic x-axis, and neither did explicit `overrides` values sent
through the chart data API. Three defects in
`QueryContextProcessor.get_viz_annotation_data`:
1. **`null` could not inherit.** The checkboxes store `{"time_range": null}`
/ `{"time_grain_sqla": null, "granularity": null}`, meaning "use the host
chart's value", but the host `QueryObject` was never passed in, so the branch
was skipped.
2. **Time range went to legacy-only fields.** It was written to `from_dttm`
/ `to_dttm`, which only the `granularity` path reads. Generic x-axis charts
carry the window in a `TEMPORAL_RANGE` filter clause.
3. **Time grain went to a legacy-only field.** It was written to
`extras["time_grain_sqla"]`. Generic x-axis charts carry the grain as
`timeGrain` on the `BASE_AXIS` column.
Changes (backend only, one module plus tests):
- `get_viz_annotation_data` takes an optional `host_query` keyword argument;
`get_annotation_data` passes its query. Existing callers and tests are
unchanged.
- The override block is replaced by module-level helpers
(`_apply_annotation_overrides` and friends) with these semantics per key:
| `overrides` entry | Meaning | Action |
|---|---|---|
| key absent | no override | source query unchanged |
| key present, value `null` | inherit from host | host's value; unchanged
when there is no host or the host has none |
| key present, non-null | explicit | that value |
- The time range is applied the way `QueryContextFactory._apply_filters`
applies a dashboard range: `query_object.time_range` is set, every
`TEMPORAL_RANGE` filter's `val` is rewritten, and `from_dttm` / `to_dttm` are
recomputed so legacy `granularity` charts keep working. A source without a
`TEMPORAL_RANGE` filter gets one on the layer's `timeColumn` when that column
exists in the source dataset.
- The time grain is applied to `extras["time_grain_sqla"]` and to the
`timeGrain` of every `BASE_AXIS` column.
- The `granularity` override key is ignored on purpose: the popover writes
it alongside `time_grain_sqla`, but it names the host dataset's time column,
which the source dataset may not have.
- `time_shift` is untouched (the ECharts plugin applies it client-side for
Time Series layers since #38126; the backend still ignores it). Known follow-up.
**Intentional behaviour change.** Override time range is checked by default,
so existing chart-sourced layers start following the host chart's time range,
including dashboard time filters, instead of the source chart's own range.
Documented in `UPDATING.md`; unchecking the box restores the old behaviour.
Open questions for reviewers:
1. **Host with "No filter":** inheriting widens a source chart that had its
own narrower range. This matches the checkbox text and the legacy client-side
behaviour. Should inheritance apply only when the host range is bounded?
2. **Interval layers:** filtering on the start column drops intervals that
start before the window but overlap it. Acceptable for this fix, or handle in a
follow-up?
3. **Feature flag:** should the behaviour change sit behind a flag for one
release? No flag is added here.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Host "Sales Linear" (Month grain, 2004) with an Event layer from "Problem
orders" (Override time range on) and a Time Series layer from a Day-grain line
chart (Override time grain on).
Before: the Time Series layer is drawn at Day grain and the Event layer
query returns all 7 orders (only the 3 inside 2004 are visible on the axis).
(screenshot: before-grain-layer.png)
After: the Time Series layer follows the host's Month grain and the Event
layer query returns the 3 orders in 2004.
(screenshot: after-explore.png)
Record counts from `POST /api/v1/chart/data` with `"force": true`:
| Case | `overrides` on the layer | Before | After |
|---|---|---|---|
| A | `{"time_range": null}` | 7 | 3 (2004) |
| B | `{"time_range": "2005-01-01 : 2006-01-01"}` | 7 | 3 (2005) |
| C | `{}` | 7 | 7 |
| G1 | `{"time_grain_sqla": null, "granularity": null}` | 117 daily | 12
monthly |
| G2 | `{"time_grain_sqla": "P1M"}` | 117 daily | 12 monthly |
| G3 | `{}` | 117 daily | 117 daily |
Dashboard-style range (the host query carrying `time_range` and
`extra_form_data.time_range` as a native Time range filter does): `2005-01-01 :
2006-01-01` returns the 3 Disputed 2005 orders, `2003-01-01 : 2004-01-01`
returns the 1 Cancelled 2003 order.
### TESTING INSTRUCTIONS
1. Load the example data. Build a Table chart "Problem orders" on
`cleaned_sales_data`: Aggregate mode, dimensions `order_date` (Day), `country`,
`order_number`, `status`, filter `status IN (Cancelled, Disputed)`, time filter
No filter (7 rows).
2. Build a Line chart "Sales Linear" on `cleaned_sales_data`: x-axis
`order_date`, Time grain Month, metric `SUM(sales)`, time range `2004-01-01 <=
col < 2005-01-01`.
3. On the host add an Event layer sourced from "Problem orders" (event time
column `order_date`), leave Override time range checked, save.
4. `POST /api/v1/chart/data` with the host's query context and `"force":
true`; count `result[0].annotation_data["Problem orders"].records` for the
A/B/C cases above.
5. For the grain: build a Line chart on `cleaned_sales_data` at Time grain
Day, add it to the host as a Time Series layer with Override time grain
checked, and check that the layer's record timestamps are month starts (12
records for 2004) instead of days.
6. Put the host on a dashboard with a Time range native filter and change
the range: the markers follow it.
7. Unit tests: `pytest
tests/unit_tests/common/test_query_context_processor.py -k annotation`.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #45796
- [ ] 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]
