aminghadersohi commented on code in PR #44893:
URL: https://github.com/apache/superset/pull/44893#discussion_r4168509831
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -2307,8 +2308,36 @@ def _validate_default_time_range(cls, v: str | None) ->
str | None:
return validate_time_range(v)
+class FilterRangeSpec(BaseNewFilterSpec):
+ """Spec for a new numerical range (filter_range) native filter."""
+
+ filter_type: Literal["filter_range"] = Field(
+ ..., description="Discriminator - must be 'filter_range'"
+ )
+ dataset_id: int = Field(..., description="ID of the dataset to filter on")
+ column: str = Field(
+ ...,
+ min_length=1,
+ description="Name of the numeric dataset column to filter on",
+ )
Review Comment:
Valid for the tool execution path. The structural schema cannot determine a
dataset column’s type by name; the dataset lookup previously checked only
existence. Fixed in ab5c0c3751b86e76fc2cfe9633bb6b6940c351e7: range-filter
creation and target updates require the resolved column’s `is_numeric`
property, matching the frontend’s numeric generic type restriction. Regression
tests reject nonnumeric columns before the update command runs, including
column-only and dataset-only updates.
Validation: `pytest tests/unit_tests/mcp_service/dashboard` — 729 passed;
CI-style pytest with dashboard and tool-inventory tests — 888 passed; staged
and branch-file pre-commit checks passed, including mypy, ruff, and pylint.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -2488,33 +2525,68 @@ class ApplyFilterValueSpec(BaseModel):
"'No filter' to clear the filter."
),
)
+ range: List[int | float | None] | None = Field(
+ None,
+ min_length=2,
+ max_length=2,
+ description=(
+ "[lower, upper] bounds to apply, for a filter_range filter. "
+ "Either bound may be null to leave that side unbounded. Pass "
+ "[null, null] to clear the filter."
+ ),
+ )
+ time_grain: List[TimeGrain] | None = Field(
+ None,
+ max_length=1,
+ description=(
+ "Time grain to apply, for a filter_timegrain filter, as a list "
+ "with at most one value, e.g. ['P1D']. Pass an empty list to "
+ "clear the filter."
+ ),
+ )
Review Comment:
Valid. `BaseEngineSpec.get_time_grains()` incorporates `TIME_GRAIN_ADDONS`,
and semantic views expose their implementation’s grain representations through
`get_time_grains()`; these are not limited to the constants enum. Fixed in
ab5c0c3751b86e76fc2cfe9633bb6b6940c351e7: both the request and applied-filter
summary accept nonempty duration strings instead of the fixed enum. Tests
round-trip custom `PT2H` and `P2D` values through the permalink and response,
while retaining the single-value limit and rejecting an empty duration string.
Validation: `pytest tests/unit_tests/mcp_service/dashboard` — 729 passed;
CI-style pytest with dashboard and tool-inventory tests — 888 passed; staged
and branch-file pre-commit checks passed, including mypy, ruff, and pylint.
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -224,6 +227,65 @@ def _time_data_mask(conf: dict[str, Any], time_range: str)
-> dict[str, Any]:
}
+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]}}
+
+ 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})
+
+ return {
+ "extraFormData": {"filters": filters},
+ "filterState": {"value": [lower, upper]},
+ }
+
+
+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)
+ if not is_set and (conf.get("controlValues") or
{}).get("enableEmptyFilter"):
+ raise _FilterApplyError(
+ f"Filter '{conf.get('name') or conf.get('id')}' requires a time "
+ "grain and cannot be cleared."
+ )
+ return {
+ "extraFormData": {"time_grain_sqla": time_grain[0]} if is_set else {},
+ "filterState": {"value": list(time_grain) if is_set else None},
+ }
Review Comment:
Valid. The saved restriction is the filter’s top-level `time_grains` field,
passed to the frontend as `timeGrains`. Fixed in
ab5c0c3751b86e76fc2cfe9633bb6b6940c351e7: a nonempty allowlist rejects an
unavailable selection before permalink creation and reports the available
grains. Missing or empty allowlists remain unrestricted; clearing an optional
filter remains supported. Regression tests cover rejected selections, allowed
custom durations, clearing, and empty/missing allowlists.
Validation: `pytest tests/unit_tests/mcp_service/dashboard` — 729 passed;
CI-style pytest with dashboard and tool-inventory tests — 888 passed; staged
and branch-file pre-commit checks passed, including mypy, ruff, and pylint.
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -152,6 +177,37 @@ def _build_new_filter_config(
"cascadeParentIds": [],
}
+ if isinstance(spec, FilterRangeSpec):
+ _validate_dataset_column(spec.dataset_id, spec.column)
Review Comment:
Valid; this is the same missing numeric-type check as the schema thread.
Fixed in ab5c0c3751b86e76fc2cfe9633bb6b6940c351e7 at the dataset lookup
boundary: `filter_range` additions and merged target updates require
`is_numeric`, while select filters retain support for nonnumeric columns. Tests
verify nonnumeric additions and column-only, dataset-only, and combined target
updates fail without invoking the persistence command.
Validation: `pytest tests/unit_tests/mcp_service/dashboard` — 729 passed;
CI-style pytest with dashboard and tool-inventory tests — 888 passed; staged
and branch-file pre-commit checks passed, including mypy, ruff, and pylint.
--
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]
