gabotorresruiz commented on code in PR #45486:
URL: https://github.com/apache/superset/pull/45486#discussion_r4394731170
##########
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:
Small NIT: I think the clause you dropped is still true.
`_apply_granularity` does remove the granularity column's `TEMPORAL_RANGE` (it
is only the non-range predicates on it that survive now), and that removal is
the first half of why a one-sided range has to be rewritten here;
`_get_time_filter` needing both bounds is the second half. Maybe keep the
sentence and just reword the parenthetical?
##########
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:
This one worries me a bit. The refusal covered two shapes and
`_apply_granularity` only stopped dropping one of them: predicates survive when
`filter_to_remove == granularity`, but when the `TEMPORAL_RANGE` sits on a
different column than the granularity, every filter on that column still goes,
which is what your own `test_apply_granularity_keeps_simple_filters` pins for
`granularity != "ds"`.
`get_table` reaches that shape on a plain dataset, because
`build_query_dict` writes `granularity` whenever `time_column` is set, even
with no `time_range`. Replaying the rewrite plus `_build_query_dict` plus
`_apply_granularity` on a dataset with `ship_date` and `order_date`: a
dashboard clause `{"col": "order_date", "op": "==", "val": 1064966400000}` with
a request of `time_column="ship_date"` and `filters=[{"col": "order_date",
"op": "TEMPORAL_RANGE", "val": "Last year"}]` comes out as `SELECT client AS
client, COUNT(*) AS count FROM orders GROUP BY client LIMIT 1000`, with no
predicate on `order_date` left at all. On master that same call is refused.
Would a narrowed guard be worth keeping for just that case? This one refuses
the mismatch while leaving every shape the PR is meant to unblock allowed,
including a dashboard range on the query's time column:
```python
temporal = {f["col"] for f in filters if f.get("op") == TEMPORAL_RANGE}
if 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.",
"Pass the time range on that column, or use the dashboard's
charts.",
)
```
`test_dataset_query_refuses_a_clause_on_a_non_time_temporal_column` next to
the rewritten test would lock it in. Or am I misreading the reachability here?
--
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]
