mikebridge commented on code in PR #45294:
URL: https://github.com/apache/superset/pull/45294#discussion_r4287680084
##########
superset/semantic_layers/mapper.py:
##########
@@ -1392,22 +1393,46 @@ def validate_filter_columns(
dimension_names: Collection[str],
metric_names: Collection[str],
) -> None:
- """Refuse chart-defined metric predicates; report incompatible extras
later."""
+ """Refuse chart-defined predicates on metrics or on columns the view lacks.
+
+ Dashboard (``isExtra``) filters are not refused: incompatible ones are
+ skipped and reported, so a dashboard filter cannot break the chart. A
+ "No filter" time range constrains nothing, so its column is not checked.
+ """
for filter_ in filters:
column: Column = filter_["col"]
if (
- isinstance(column, str)
- and column not in dimension_names
- and column in metric_names
- and not filter_.get("isExtra")
+ not isinstance(column, str)
+ or column in dimension_names
+ or filter_.get("isExtra")
+ or _is_no_op_time_range(filter_)
Review Comment:
Fixed in
https://github.com/apache/superset/commit/224a5fbded0ad75f01273b4d3d4eb9cdc4a7470e.
The overwrite is in `QueryContextFactory._apply_filters`
(`superset/common/query_context_factory.py:450`): browser dashboard time
overrides now retain `isExtra` when that value is applied, and server dashboard
context stamps it before the rewrite. Red-first HTTP cases reproduced the 400
for both paths; both now render with the foreign column reported as
incompatible and no provider predicate. Chart-owned and mismatched overrides
still refuse. Implemented and verified with Codex.
##########
superset/semantic_layers/mapper.py:
##########
@@ -1392,22 +1393,46 @@ def validate_filter_columns(
dimension_names: Collection[str],
metric_names: Collection[str],
) -> None:
- """Refuse chart-defined metric predicates; report incompatible extras
later."""
+ """Refuse chart-defined predicates on metrics or on columns the view lacks.
+
+ Dashboard (``isExtra``) filters are not refused: incompatible ones are
+ skipped and reported, so a dashboard filter cannot break the chart. A
+ "No filter" time range constrains nothing, so its column is not checked.
+ """
for filter_ in filters:
column: Column = filter_["col"]
if (
- isinstance(column, str)
- and column not in dimension_names
- and column in metric_names
- and not filter_.get("isExtra")
+ not isinstance(column, str)
+ or column in dimension_names
+ or filter_.get("isExtra")
+ or _is_no_op_time_range(filter_)
):
+ continue
+ if column in metric_names:
raise QueryObjectValidationError(
_(
"Filter column '%(column)s' is a metric; semantic views
only "
- "filter on dimensions",
+ "filter on dimensions. Select a dimension or remove the
filter.",
column=column,
)
)
+ raise QueryObjectValidationError(
Review Comment:
Thanks, applied your ordering in
https://github.com/apache/superset/commit/224a5fbded0ad75f01273b4d3d4eb9cdc4a7470e:
metric refusal runs before the missing-column no-op exemption. Added red-first
HTTP tests for canonical, localized and null temporal no-ops on `total_amount`;
all return 400 without querying the provider. Missing-column no-ops still
render. 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:
Thanks for spelling out the full deploy window. Mike accepted keeping the
400 for this opt-in, pre-GA feature (decision D24), with this compatibility
decision to be revisited before GA.
https://github.com/apache/superset/commit/224a5fbded0ad75f01273b4d3d4eb9cdc4a7470e
and the PR body now explicitly say that only `SEMANTIC_LAYERS`-enabled
deployments are affected: dashboard tabs already open across the upgrade send
unmarked filters, and their semantic charts return 400 for incompatible
dashboard filters until the user reloads the page. Documentation updated 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]
