aminghadersohi commented on code in PR #45486:
URL: https://github.com/apache/superset/pull/45486#discussion_r4394894470
##########
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:
You're right; I reproduced that exact `get_table` shape at
`19cf733704088dbddeec2c96899cba4a6d9d8386`. After scope rewriting both
`order_date` filters are present, but `_apply_granularity` leaves an empty
filter list and SQL with no WHERE:
```sql
SELECT client AS client, COUNT(*) AS count
FROM orders GROUP BY client
LIMIT 1000 OFFSET 0
```
Fixed in `a72613f8422e8b31d3073cf3bc61e8028409a4bb` with the narrowed guard.
It examines the combined model/dashboard clauses and skips the refusal when an
explicit `time_range` appends a matching binding. Same-column dashboard
comparisons and ranges remain allowed.
Regression test:
`test_dataset_query_refuses_a_clause_on_a_non_time_temporal_column`
(`tests/unit_tests/mcp_service/test_dashboard_scope.py`), parametrized over
both dataset tools. Restoring the reviewed head's `dashboard_scope.py` gives
**2 failed** (`DID NOT RAISE`); the new head gives **2 passed**.
`test_dataset_query_keeps_scope_clauses_with_safe_temporal_bindings` adds **8
passing controls** for the allowed shapes, including a dashboard range on the
query's time column. I ran the four touched test files at the fixing SHA: **480
passed**, with `manual`/`llm_compliance` deselected; changed-file pre-commit
also passed. The PR description includes the command and the refreshed base
comparison: **149 passed, 1 failed** without the production diff, **150
passed** at this head for the original regression selection.
Siblings checked:
- superset/mcp_service/dashboard_scope.py:1018 — fixed in
a72613f8422e8b31d3073cf3bc61e8028409a4bb
- superset/mcp_service/semantic_layer/tool/get_table.py:137 — fixed in
a72613f8422e8b31d3073cf3bc61e8028409a4bb via the shared dataset-scope rewrite;
semantic-view requests remain refused while scoped.
- superset/mcp_service/dataset/tool/query_dataset.py:244 — not affected:
without a request range it does not set granularity; with a range it appends a
matching temporal binding. The regression also pins the conservative
shared-scope refusal for this tool.
- superset/mcp_service/dashboard_scope.py:931 — not affected: chart
data/preview and dashboard-data paths compose chart extra-form-data and retain
their existing temporal-clause refusal, rather than using the dataset builder.
- superset/mcp_service/dashboard_scope_sql.py:485 — not affected: SQL scope
injects SQL predicates directly, without QueryContextFactory granularity
rewriting.
- superset/common/tabular_query.py:326 — not affected: the builder
intentionally sets explicit granularity; its scoped dataset callers run through
the shared guard before reaching it.
- superset/common/query_context_factory.py:287 — not affected: cross-column
removal remains intentional for overrides; this change refuses the unsafe
scoped dataset request before that removal.
- superset/common/form_data_query_context.py:1017 — not affected: chart-axis
normalization, not a separate scoped dataset entry point.
- superset/common/query_object.py:690 — not affected: temporal-bound
cache-key handling does not perform the cross-column removal.
- superset/common/query_actions.py:92 — not affected: filter-status
reporting does not construct or replace predicates.
- superset/mcp_service/chart/chart_helpers.py:1266 — not affected: shared
chart normalization; scoped chart calls use chart extra-form-data composition.
- superset/mcp_service/chart/chart_utils.py:3007 — not affected: chart
generation/update builders, not the dataset-scope entry point; scoped data
previews and saved-definition updates are refused by their existing gates.
- superset/mcp_service/chart/plugins/big_number.py:221 — not affected: chart
configuration/time-binding inheritance, not the dataset rewrite; existing scope
gates refuse data previews and saved-definition updates.
- superset/mcp_service/chart/plugins/gantt.py:152 — not affected: a
temporal-column validation error, not a query rewrite.
- superset/mcp_service/chart/plugins/geographic.py:146 — not affected: chart
granularity binding; scoped chart reads use existing extra-form-data checks,
not the dataset builder.
- superset/mcp_service/chart/plugins/waterfall.py:213 — not affected: chart
normalization, not a separate scoped dataset entry point.
- superset/mcp_service/chart/schemas.py:3491 — not affected: inherited chart
time-column metadata only.
- superset/mcp_service/dataset/schemas.py:1259 — not affected: request-field
declaration only.
- superset/mcp_service/semantic_layer/schemas.py:252 — not affected:
request-field declarations only.
Search: `grep -rn --include='*.py' 'time_column\|_apply_granularity'
superset/mcp_service superset/common`; entries above group repeated references
within each path.
##########
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:
I agree. The docstring at `a72613f8422e8b31d3073cf3bc61e8028409a4bb` again
explains both steps: `_apply_granularity` removes the granularity column's
`TEMPORAL_RANGE`, while non-range predicates survive when the time column is
unchanged; the semantic mapper then requires both bounds.
This nit changes documentation only, so there is no fail-before runtime
behavior to claim for it. A docstring-content check finds both
`_apply_granularity` and `_get_time_filter` at this head, but not at
`19cf733704088dbddeec2c96899cba4a6d9d8386`.
`test_apply_granularity_keeps_simple_filters` covers the described range
removal/predicate retention; the one-sided-range selection in
`tests/unit_tests/common/test_tabular_query.py` passes **3 tests** at the new
head. The separate behavioral regression in the same commit,
`test_dataset_query_refuses_a_clause_on_a_non_time_temporal_column`, fails both
cases before the guard and passes both afterward, as detailed in the other
thread.
Siblings checked:
- superset/common/tabular_query.py:285 — fixed in
a72613f8422e8b31d3073cf3bc61e8028409a4bb
- superset/common/query_context_factory.py:435 — not affected: its existing
removal behavior is the first step described by this comment; no runtime change
here.
- superset/semantic_layers/mapper.py:777 — not affected: its existing
requirement for both bounds is the second step described by the comment; no
runtime change 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]
