mikebridge commented on code in PR #45294:
URL: https://github.com/apache/superset/pull/45294#discussion_r4289532990
##########
superset/common/query_context_factory.py:
##########
@@ -434,8 +434,19 @@ def _apply_granularity( # noqa: C901
if filter["col"] != filter_to_remove
]
- def _apply_filters(self, query_object: QueryObject) -> None:
+ def _apply_filters(
+ self, query_object: QueryObject, form_data: dict[str, Any] | None =
None
+ ) -> None:
+ """Apply the time range, retaining the source of dashboard
overrides."""
+ extra_form_data: object = (form_data or {}).get("extra_form_data")
+ dashboard_time_override: bool = (
+ isinstance(extra_form_data, dict)
+ and "time_range" in extra_form_data
+ and extra_form_data["time_range"] == query_object.time_range
+ )
if query_object.time_range:
for filter_object in query_object.filter:
if filter_object["op"] == "TEMPORAL_RANGE":
filter_object["val"] = query_object.time_range
+ if dashboard_time_override:
+ filter_object["isExtra"] = True
Review Comment:
Thanks, you are right: the previous stamp was too broad. Fixed in
https://github.com/apache/superset/commit/a8fbfb85918a52046c86fb58f7796566b34d1d19:
`_apply_filters` checks the saved value before rewriting and only adds
`isExtra` for None or canonical/localized No filter. Bounded chart predicates
on missing columns and metrics stay refused. Red-first
`test_dashboard_time_override_preserves_foreign_filter_provenance` now includes
your three rows, and
`test_time_override_stamps_only_unbounded_saved_predicates` pins the shared
rule and preservation of existing markers in the common suite. Required/related
suites: 1,705 passed; branch-file hooks passed. Implemented and verified with
Codex.
##########
superset/charts/data/dashboard_filter_context.py:
##########
@@ -382,5 +383,11 @@ def apply_dashboard_filter_context( # noqa: C901
for src_key, target_key in
EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS.items():
if src_key in extra_form_data:
query[target_key] = extra_form_data[src_key]
+ if src_key == "time_range" and extra_form_data[src_key]:
+ # The query factory rewrites temporal predicates with this
range.
+ filter_: QueryObjectFilterClause
+ for filter_ in query.get("filters") or []:
+ if filter_.get("op") == "TEMPORAL_RANGE":
+ filter_["isExtra"] = True
Review Comment:
Fixed the same saved-value guard in `apply_dashboard_filter_context` in
https://github.com/apache/superset/commit/a8fbfb85918a52046c86fb58f7796566b34d1d19,
so a bounded chart predicate is not newly marked by either the chart-data or
Excel-workbook caller (`workbook.py:223`). The server rows of
`test_dashboard_time_override_preserves_foreign_filter_provenance` reproduced
both bounded regressions red-first and now refuse400; canonical/localized/null
unbounded cases still render. The related Excel-workbook suite also passed.
Implemented and verified with Codex.
##########
UPDATING.md:
##########
@@ -71,8 +71,16 @@ assists people when migrating to a new version.
the metric; select a dimension or remove the unsupported predicate. Dashboard
filters on incompatible columns (including metrics) are skipped and reported
as rejected, so the chart renders with an **Incompatible** filter indicator.
- Unknown chart-defined columns are also reported as rejected, matching SQL
- datasets. Valid dimension filters are reported as applied.
+ A chart-defined filter on a column the semantic view no longer has also
+ returns a client error naming the column, instead of being skipped (which
+ widened the result); edit the chart's filters and select a dimension. This
+ differs from SQL datasets, which skip and report such filters. Scheduled
+ reports, alerts and thumbnails for such charts fail until the chart is
edited.
+ A "No filter" time range is not refused. Reload open dashboards after
deploying:
+ older browser bundles do not mark dashboard filters with `isExtra`, so their
+ incompatible dashboard filters would be refused as chart filters. A dashboard
Review Comment:
Updated that sentence in
https://github.com/apache/superset/commit/a8fbfb85918a52046c86fb58f7796566b34d1d19:
dashboard time overrides mark only previously unbounded temporal predicates;
bounded chart-defined filters on missing columns or metrics remain refused.
`test_dashboard_time_override_preserves_foreign_filter_provenance` and
`test_time_override_stamps_only_unbounded_saved_predicates` now pin that
distinction. The accepted pre-GA D24 deploy window and revisit-before-GA note
remain unchanged. Updated and verified with Codex.
--
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]
