gabotorresruiz commented on code in PR #44893:
URL: https://github.com/apache/superset/pull/44893#discussion_r4170680038
##########
tests/unit_tests/mcp_service/test_tool_inventory.py:
##########
@@ -53,7 +53,7 @@ def budgeted_bytes(text: str) -> int:
TOOL_BUDGETS = {
"add_chart_to_existing_dashboard": 1_500,
- "apply_dashboard_filters": 2_900,
+ "apply_dashboard_filters": 3_900,
Review Comment:
Just a small NIT: the rule this file states is `ceil(measured / 100) * 100 +
100`, and I measured this entry at 3598 bytes on head, which gives 3700.
`manage_native_filters` below follows it exactly (8793 measured, 8900 set);
this one leaves 302 bytes of headroom instead of 102.
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -224,6 +228,123 @@ def _time_data_mask(conf: dict[str, Any], time_range:
str) -> dict[str, Any]:
}
+def _format_range_bound(value: int | float) -> str:
+ """Format a range bound, preserving finite values that overflow
rounding."""
+ try:
+ return format_smart_number(value)
+ except OverflowError:
+ return repr(value)
+
+
+def _range_data_mask(
+ conf: dict[str, Any], bounds: list[int | float | None]
+) -> dict[str, Any]:
+ """Build the data mask a filter_range filter produces for ``bounds``.
+
+ Mirrors the frontend's ``getRangeExtraFormData``: distinct non-null
+ bounds become a pair of ``>=``/``<=`` predicates, equal non-null bounds
+ collapse to a single ``==`` predicate, and two null bounds produce no
+ predicate at all (the "required filter, nothing chosen" state when the
+ filter is marked ``enableEmptyFilter``, raised rather than guessed).
+ """
+ targets = [target for target in (conf.get("targets") or []) if target]
+ column = (targets[0].get("column") or {}).get("name") if targets else None
+ if not column:
+ raise _FilterApplyError(
+ f"Filter '{conf.get('name') or conf.get('id')}' has no target "
+ "column, so a value cannot be applied to it."
+ )
+
+ lower, upper = bounds
+ if lower is None and upper is None:
+ if (conf.get("controlValues") or {}).get("enableEmptyFilter"):
+ raise _FilterApplyError(
+ f"Filter '{conf.get('name') or conf.get('id')}' requires a "
+ "value and cannot be cleared."
+ )
+ return {
+ "extraFormData": {},
+ "filterState": {"value": [None, None], "label": ""},
+ }
+
+ filters: list[dict[str, Any]] = []
+ if lower == upper:
+ filters.append({"col": column, "op": "==", "val": upper})
+ else:
+ if lower is not None:
+ filters.append({"col": column, "op": ">=", "val": lower})
+ if upper is not None:
+ filters.append({"col": column, "op": "<=", "val": upper})
Review Comment:
Small question: should this consult `controlValues.enableSingleValue`? A
range filter saved in `max` mode can only ever produce a `<=` predicate in the
UI, because the plugin builds `[null, singleValue]` for that mode
(`RangeFilterPlugin.tsx:485`), but the tool accepts `range: [10, 100]` on it
and writes both `>=` and `<=`; I confirmed that on this branch.
`NativeFilterSummary` does not surface `controlValues` either, so the caller
has no way to tell the filter is in single-value mode. Or am I misunderstanding
how that mode is meant to interact with a stored mask?
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -152,6 +186,37 @@ def _build_new_filter_config(
"cascadeParentIds": [],
}
+ if isinstance(spec, FilterRangeSpec):
+ _validate_dataset_column(spec.dataset_id, spec.column,
require_numeric=True)
+ return {
+ "id": filter_id,
+ "type": "NATIVE_FILTER",
+ "filterType": "filter_range",
+ "name": spec.name,
+ "description": spec.description,
+ "scope": scope,
+ "targets": [
+ {"datasetId": spec.dataset_id, "column": {"name": spec.column}}
+ ],
+ "controlValues": {"enableEmptyFilter": spec.enable_empty_filter},
+ "defaultDataMask": _empty_data_mask(),
+ "cascadeParentIds": [],
+ }
+
+ if isinstance(spec, FilterTimeGrainSpec):
+ return {
+ "id": filter_id,
+ "type": "NATIVE_FILTER",
+ "filterType": "filter_timegrain",
+ "name": spec.name,
+ "description": spec.description,
+ "scope": scope,
+ "targets": [{}],
Review Comment:
This block worries me a bit. `targets: [{}]` is copied from the
`filter_time` return below, but `filter_time` is the only genuinely
dataset-less native filter type: its plugin sets `datasourceCount: 0`
explicitly (`superset-frontend/src/filters/components/Time/index.ts:34`), while
`TimeGrain/index.ts` sets no count at all, so it defaults to `1`
(`ChartMetadata.ts:120`). That makes `hasDataset` true for `filter_timegrain`
in the config modal (`FiltersConfigForm.tsx:396`), and `hasFilledDataset` then
requires a `datasetId` before a save is allowed (`:474`). The plugin needs it
too: `buildQuery.ts` issues a `result_type: 'timegrains'` request against that
dataset to get its grain list.
The consequence is that the two halves of this PR do not compose. I chained
them on this branch: `manage_native_filters` produced `{"filterType":
"filter_timegrain", ..., "targets": [{}], "controlValues":
{"enableEmptyFilter": false}, ...}`, and feeding that exact config to
`apply_dashboard_filters` with `{"time_grain": ["P1D"]}` fails with
```
Cannot resolve target dataset (ID None) for filter 'Granularity'; supported
time grains cannot be determined.
```
from `_timegrain_data_mask` (`apply_dashboard_filters.py:314`). There is
also no repair path: `_TYPE_SPECIFIC_UPDATE_FIELDS["filter_timegrain"]` is
`{"enable_empty_filter"}` (line 76), so `dataset_id` and `column` are rejected
on update, which `test_update_dataset_field_on_timegrain_filter_rejected`
asserts on purpose. An agent that creates a time grain filter with this tool
can never set a value on it with the other one.
It is broken in the filter bar as well. With no `datasetId`, `FilterValue`
skips the chart data request entirely (`FilterValue.tsx:283`) and passes
`queriesDataPlaceholder = [{ data: [{}] }]` to the chart (`:511`), which
`transformProps` hands straight through, so `durationMap` is built from `[{}]`
and the dropdown renders one option with no label
(`TimeGrainFilterPlugin.tsx:54` and `:104`).
The fix I would expect is a required `dataset_id` on `FilterTimeGrainSpec`,
emitted here as `"targets": [{"datasetId": spec.dataset_id}]`, plus
`dataset_id` added to the `filter_timegrain` entry of
`_TYPE_SPECIFIC_UPDATE_FIELDS`. Note `_merge_target` currently insists on both
`dataset_id` and `column`, so it would need to become type aware, since a time
grain filter has no column.
For tests: tighten `test_add_filter_timegrain` to assert the `datasetId`
instead of `[{}]`, and add a chained one, say
`test_created_timegrain_filter_accepts_a_grain`, that feeds the config
`manage_native_filters` emits into `apply_dashboard_filters`. The reason the
current 933 tests miss this is that each side is pinned to a shape the other
never produces: `test_apply_dashboard_filters.py`'s `TIMEGRAIN_FILTER`
hardcodes `"targets": [{"datasetId": 5}]`, and
`test_timegrain_without_allowlist_rejects_unresolvable_dataset` even
parametrizes `[{}]` as an expected rejection, while
`test_manage_native_filters.py` asserts `config["targets"] == [{}]`.
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -224,6 +228,123 @@ def _time_data_mask(conf: dict[str, Any], time_range:
str) -> dict[str, Any]:
}
+def _format_range_bound(value: int | float) -> str:
+ """Format a range bound, preserving finite values that overflow
rounding."""
+ try:
+ return format_smart_number(value)
+ except OverflowError:
+ return repr(value)
+
+
+def _range_data_mask(
+ conf: dict[str, Any], bounds: list[int | float | None]
+) -> dict[str, Any]:
+ """Build the data mask a filter_range filter produces for ``bounds``.
+
+ Mirrors the frontend's ``getRangeExtraFormData``: distinct non-null
+ bounds become a pair of ``>=``/``<=`` predicates, equal non-null bounds
+ collapse to a single ``==`` predicate, and two null bounds produce no
+ predicate at all (the "required filter, nothing chosen" state when the
+ filter is marked ``enableEmptyFilter``, raised rather than guessed).
+ """
+ targets = [target for target in (conf.get("targets") or []) if target]
+ column = (targets[0].get("column") or {}).get("name") if targets else None
+ if not column:
+ raise _FilterApplyError(
+ f"Filter '{conf.get('name') or conf.get('id')}' has no target "
+ "column, so a value cannot be applied to it."
+ )
+
+ lower, upper = bounds
+ if lower is None and upper is None:
+ if (conf.get("controlValues") or {}).get("enableEmptyFilter"):
+ raise _FilterApplyError(
+ f"Filter '{conf.get('name') or conf.get('id')}' requires a "
+ "value and cannot be cleared."
+ )
+ return {
+ "extraFormData": {},
+ "filterState": {"value": [None, None], "label": ""},
+ }
+
+ filters: list[dict[str, Any]] = []
+ if lower == upper:
+ filters.append({"col": column, "op": "==", "val": upper})
+ else:
+ if lower is not None:
+ filters.append({"col": column, "op": ">=", "val": lower})
+ if upper is not None:
+ filters.append({"col": column, "op": "<=", "val": upper})
+
+ if lower == upper:
+ assert upper is not None
+ label = f"x = {_format_range_bound(upper)}"
+ elif lower is not None and upper is not None:
+ label = f"{_format_range_bound(lower)} ≤ x ≤
{_format_range_bound(upper)}"
+ elif lower is not None:
+ label = f"x ≥ {_format_range_bound(lower)}"
+ else:
+ assert upper is not None
+ label = f"x ≤ {_format_range_bound(upper)}"
+ return {
+ "extraFormData": {"filters": filters},
+ "filterState": {"value": [lower, upper], "label": label},
+ }
+
+
+def _timegrain_data_mask(
+ conf: dict[str, Any], time_grain: Sequence[str]
+) -> dict[str, Any]:
+ """Build the data mask a filter_timegrain filter produces for
``time_grain``."""
+ is_set = bool(time_grain)
+ allowed_grains = conf.get("time_grains")
+ if is_set and allowed_grains and time_grain[0] not in allowed_grains:
+ raise _FilterApplyError(
+ f"Time grain '{time_grain[0]}' is not allowed for filter "
+ f"'{conf.get('name') or conf.get('id')}'. "
+ f"Available time grains: {', '.join(allowed_grains)}."
+ )
+ filter_state: dict[str, Any] = {"value": list(time_grain) if is_set else
None}
+ if is_set:
+ from superset.daos.dataset import DatasetDAO
+
+ targets = [target for target in (conf.get("targets") or []) if target]
+ dataset_id = targets[0].get("datasetId") if targets else None
+ dataset = DatasetDAO.find_by_id(dataset_id) if dataset_id is not None
else None
+ if dataset is None and not allowed_grains:
+ raise _FilterApplyError(
+ f"Cannot resolve target dataset (ID {dataset_id}) for filter "
+ f"'{conf.get('name') or conf.get('id')}'; "
+ "supported time grains cannot be determined."
+ )
+ # Reuse the datasource options exposed to Explore and native filters.
+ available_grains = [
+ duration
+ for duration, _ in (dataset.time_grain_sqla if dataset is not None
else [])
+ if duration is not None
+ ]
+ if not allowed_grains and time_grain[0] not in available_grains:
Review Comment:
Not a blocker, but these two branches disagree with each other. With no
`time_grains` allowlist the selection is validated against the dataset's own
grains; with one, this dataset check is skipped completely and only the
allowlist is consulted at line 301.
I verified it on this branch: a filter with `time_grains: ["PT6H"]` whose
target dataset is Postgres accepts `PT6H` and writes `{"time_grain_sqla":
"PT6H"}`, while the same selection with the allowlist removed is rejected with
`Available time grains: ... PT1H, P1D, P1W, P1M, P3M, P1Y`. Postgres has no
`PT6H` grain spec, so `get_timestamp_expr` raises `NotImplementedError: No
grain spec for PT6H for database postgresql` once a chart renders with it. Same
shape for `P2D` on SQLite.
The plugin intersects the two sets rather than preferring one: its `options`
come from the datasource query and are then filtered by the allowlist
(`TimeGrainFilterPlugin.tsx:104`), so the UI would never offer that grain.
Worth dropping the `not allowed_grains` guard so an allowlisted grain still has
to appear in `available_grains`? An allowlist only drifts from its dataset when
the dataset is repointed at another database or the dashboard is imported, so
this is cheap insurance rather than something a user hits today.
--
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]
