gabotorresruiz commented on code in PR #45294:
URL: https://github.com/apache/superset/pull/45294#discussion_r4286677699
##########
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:
Not a blocker, and I see the note is already here, but I want to check we
are sizing this window deliberately, because I think it is wider than it reads.
I measured the stale bundle shape on this branch against `0ce9b6c`: a
dashboard native filter `{"col": "country", "op": "IN", "val": ["US"]}` with no
`isExtra` key returns `200` on the base with `country` in
`rejected_filter_columns`, and `400` here. `isExtra` on dashboard filters only
landed in #45105, which is in this same unreleased window, so on upgrade from
any current release every dashboard tab open across the deploy sends exactly
that shape until someone reloads it. And a dashboard filter on a column that is
not a dimension of the view is the normal cross dataset case rather than a
misconfiguration: the paragraph this PR adds to
`creating-your-first-dashboard.mdx` says so. So in that window a healthy
dashboard turns every in scope semantic chart into a `400` that tells the user
to edit chart filters which are innocent. #45105 called this out only for a
dashboard filter whose name collides with a metric, which is a much smaller
target than any column the view lacks.
`Reload open dashboards after deploying` is also not something an operator
can do on a user's behalf. Since the absence of `isExtra` is indistinguishable
between "the chart owns this filter" and "old client", is there something
request level we could key off instead, so an old payload degrades to the
Incompatible path rather than erroring? `apply_dashboard_filter_context`
already stamps `isExtra` server side at
`superset/charts/data/dashboard_filter_context.py:364` for the
`filters_dashboard_id` route, so server owned provenance is already a shape in
the codebase. Or am I overweighting the window given `SEMANTIC_LAYERS` is still
opt in?
##########
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:
Small one, and possibly intentional: `_is_no_op_time_range` sitting in the
`continue` guard also switches off the pre existing metric refusal. `{"col":
"total_amount", "op": "TEMPORAL_RANGE", "val": "No filter"}` with no `isExtra`
returns `400` on `0ce9b6c` and `200` here, same with `"val": null`. Nothing
widens so it is harmless, but it does make the first sentence of that
`UPDATING.md` bullet untrue for that shape. Checking the no-op below the metric
branch keeps both:
```suggestion
if (
not isinstance(column, str)
or column in dimension_names
or filter_.get("isExtra")
):
continue
if column in metric_names:
raise QueryObjectValidationError(
_(
"Filter column '%(column)s' is a metric; semantic views
only "
"filter on dimensions. Select a dimension or remove the
filter.",
column=column,
)
)
if _is_no_op_time_range(filter_):
continue
raise QueryObjectValidationError(
```
I tried that locally and the whole of `metric_filter_test.py` still passes,
with the metric no-op case back to `400`. A no-op temporal range on
`total_amount` is the case to add there to pin it.
--
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]
