gabotorresruiz commented on code in PR #45486:
URL: https://github.com/apache/superset/pull/45486#discussion_r4403564696
##########
superset/mcp_service/dashboard_scope.py:
##########
@@ -1032,22 +1015,19 @@ def scoped_dataset_query(
time_range = intersect_time_ranges(constraints.time_range, time_range)
time_column = scope_column
- granularity = (time_column or main_dttm_col) if time_range else time_column
final = [*filters, *scope_filters]
- if time_range and granularity:
- final_with_time = [
- *final,
- {"col": granularity, "op": TEMPORAL_RANGE, "val": time_range},
- ]
- else:
- final_with_time = final
- removed = removed_granularity_column(final_with_time, granularity)
- if removed is not None and removed in constraints.columns:
- raise MCPDashboardScopeError(
- f"the dashboard filter on {removed!r} would be dropped because
that "
- "column is also the query's time column.",
- "Query without a time range on that column, or use the dashboard's
charts.",
- )
+ temporal = {f["col"] for f in final if f.get("op") == TEMPORAL_RANGE}
+ # An explicit range appends a TEMPORAL_RANGE on the query's time column.
+ # Without it, granularity can replace a different column's temporal filter
+ # and discard every dashboard predicate on that column.
+ if not time_range and temporal and time_column and time_column not in
temporal:
+ if dropped := sorted(temporal & constraints.columns):
Review Comment:
Not a blocker and possibly out of scope for this PR: because of the `&
constraints.columns` intersection, the refusal only covers columns the
dashboard filters. When the `TEMPORAL_RANGE` is a caller filter on a temporal
column the dashboard does not filter, `get_table` still drops every predicate
on that column and the window moves onto `time_column` instead; I see the same
on master, so it is not something you introduce here. Worth widening, or do you
read an explicit `time_column` as the caller asking for exactly that override?
##########
superset/mcp_service/dashboard_scope.py:
##########
@@ -1032,22 +1015,19 @@ def scoped_dataset_query(
time_range = intersect_time_ranges(constraints.time_range, time_range)
time_column = scope_column
- granularity = (time_column or main_dttm_col) if time_range else time_column
final = [*filters, *scope_filters]
- if time_range and granularity:
- final_with_time = [
- *final,
- {"col": granularity, "op": TEMPORAL_RANGE, "val": time_range},
- ]
- else:
- final_with_time = final
- removed = removed_granularity_column(final_with_time, granularity)
- if removed is not None and removed in constraints.columns:
- raise MCPDashboardScopeError(
- f"the dashboard filter on {removed!r} would be dropped because
that "
- "column is also the query's time column.",
- "Query without a time range on that column, or use the dashboard's
charts.",
- )
+ temporal = {f["col"] for f in final if f.get("op") == TEMPORAL_RANGE}
+ # An explicit range appends a TEMPORAL_RANGE on the query's time column.
+ # Without it, granularity can replace a different column's temporal filter
+ # and discard every dashboard predicate on that column.
+ if not time_range and temporal and time_column and time_column not in
temporal:
+ if dropped := sorted(temporal & constraints.columns):
+ raise MCPDashboardScopeError(
+ f"the dashboard filter on {dropped[0]!r} would be dropped
because "
+ f"the query's time range is on {time_column!r} instead.",
Review Comment:
Tiny wording NIT: the guard only fires when there is no `time_range`, so at
this point the query has no time range at all and what sits on `time_column` is
the granularity.
```suggestion
f"the query's time column is {time_column!r} instead.",
```
##########
superset/mcp_service/dashboard_scope.py:
##########
@@ -1032,23 +1015,7 @@ def scoped_dataset_query(
time_range = intersect_time_ranges(constraints.time_range, time_range)
time_column = scope_column
- granularity = (time_column or main_dttm_col) if time_range else time_column
- final = [*filters, *scope_filters]
- if time_range and granularity:
- final_with_time = [
- *final,
- {"col": granularity, "op": TEMPORAL_RANGE, "val": time_range},
- ]
- else:
- final_with_time = final
- removed = removed_granularity_column(final_with_time, granularity)
- if removed is not None and removed in constraints.columns:
- raise MCPDashboardScopeError(
- f"the dashboard filter on {removed!r} would be dropped because
that "
- "column is also the query's time column.",
- "Query without a time range on that column, or use the dashboard's
charts.",
- )
- return final, time_range, time_column
+ return [*filters, *scope_filters], time_range, time_column
Review Comment:
Confirmed at `a72613f8`: that `get_table` shape is refused again, and the
shapes the PR unblocks still carry the dashboard clause into the generated SQL.
Thanks for the fast turnaround.
One forward looking note, not for this PR: the skip condition in
`scoped_dataset_query` and `_apply_granularity`'s retention condition are two
separate predicates in two files that have to agree, and the eight controls
assert what `scoped_dataset_query` returns rather than what the query ends up
being. One case that drives a scoped `get_table` request through to the
generated query and asserts the dashboard predicate is still in it would be
cheap insurance against those two drifting apart.
##########
superset/common/tabular_query.py:
##########
@@ -287,10 +287,8 @@ def _time_range_filters(
"""Express *time_range* as query filters.
Semantic views need a one-sided range rewritten as an explicit comparison:
- ``_apply_granularity`` removes the granularity column's ``TEMPORAL_RANGE``
- filter (retaining non-range predicates on that column), and the mapper's
- ``_get_time_filter`` emits nothing unless both bounds resolve, so the range
- would vanish and the query would scan the whole view.
+ The mapper's ``_get_time_filter`` emits nothing unless both bounds resolve,
Review Comment:
Agreed, the new parenthetical reads right: `_apply_granularity` removes the
granularity column's `TEMPORAL_RANGE` and keeps the non-range predicates on it.
Closing this one.
--
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]
