aminghadersohi commented on code in PR #44893:
URL: https://github.com/apache/superset/pull/44893#discussion_r4170843515
##########
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:
Fixed in 606d7f022f2a3ca323cb1b796b06e83153b111e2: `FilterTimeGrainSpec` now
requires `dataset_id`, `_build_new_filter_config` emits `targets:
[{"datasetId": spec.dataset_id}]` (validated against the DAO at creation),
`_TYPE_SPECIFIC_UPDATE_FIELDS["filter_timegrain"]` now allows `dataset_id`, and
`_merge_target` is type-aware (a time grain filter has a dataset but no column,
unlike select/range). Tightened `test_add_filter_timegrain` to assert the real
target shape, added `test_add_filter_timegrain_with_invalid_dataset` and
`test_update_timegrain_dataset_target`, and added the chained regression test
you asked for:
`test_timegrain_filter_created_by_manage_native_filters_accepts_a_grain` in
test_apply_dashboard_filters.py, which creates a filter_timegrain filter via
manage_native_filters and feeds the exact emitted config into
apply_dashboard_filters, asserting a grain applies successfully.
##########
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:
Fixed in 606d7f022f2a3ca323cb1b796b06e83153b111e2: dropped the `not
allowed_grains` guard on both the dataset-resolution check and the
dataset-grain availability check in `_timegrain_data_mask`, so an allowlisted
grain now also has to appear in the dataset's own grains, regardless of whether
an allowlist is set. Added
`test_timegrain_in_allowlist_but_unsupported_by_dataset_is_rejected`, which
reproduces your PT6H-on-Postgres case. This also required updating
`test_timegrain_allowlist_accepts_allowed_custom_and_clear_values` (renamed to
`..._accepts_dataset_supported_and_clear_values`): two of its parametrized
cases (P2D, PT2H) were pinning the old bypass behavior with grains SQLite
doesn't natively support, so I dropped those and kept only the
dataset-supported P1D cases.
##########
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:
Implemented in 606d7f022f2a3ca323cb1b796b06e83153b111e2: added
`_validate_single_value_mode`, called from `_range_data_mask` once a clear is
ruled out. It checks `controlValues.enableSingleValue` (mirroring the
frontend's `SingleValueType` enum: 0=Minimum, 1=Exact, 2=Maximum) and rejects a
bounds shape that doesn't match the configured mode (an upper bound in Minimum
mode, a lower bound in Maximum mode, or mismatched bounds in Exact mode)
instead of silently writing both predicates. Added six tests covering the
accept/reject path for all three modes. I did not also expand
`NativeFilterSummary` to surface `controlValues` — that's a pre-existing,
deliberate omission across every filter type (per its own docstring: "omitting
verbose fields"), not something this PR's scope changed, and widening it would
need its own budget-conscious pass across all filter types rather than a
one-off for range.
##########
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:
Fixed in 606d7f022f2a3ca323cb1b796b06e83153b111e2: tightened
`apply_dashboard_filters` to 3,700 (measured 3,598, matches the stated formula)
and bumped `manage_native_filters` to 9,100 (measured 8,970 after the
`dataset_id` field addition above, also matches the formula with ~130 bytes
headroom).
--
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]
