codeant-ai-for-open-source[bot] commented on code in PR #44985:
URL: https://github.com/apache/superset/pull/44985#discussion_r4188044033
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -64,6 +72,102 @@ def _empty_data_mask() -> dict[str, Any]:
return {"filterState": {"value": None}, "extraFormData": {}}
+def _value_label(value: FilterSelectValue) -> str:
+ """Format one selected value the way the dashboard UI labels it."""
+ if value is None:
+ return _NULL_LABEL
+ if isinstance(value, bool):
+ return _TRUE_LABEL if value else _FALSE_LABEL
+ return str(value)
+
+
+def _select_data_mask(
+ conf: dict[str, Any], values: list[FilterSelectValue]
+) -> dict[str, Any]:
+ """Build the data mask a filter_select filter produces for ``values``.
+
+ Mirrors the frontend's ``getSelectExtraFormData``: a non-empty selection
+ becomes an ``IN`` predicate on the filter's target column, and an empty
+ selection on a filter marked ``enableEmptyFilter`` becomes an impossible
+ predicate (the "required filter, nothing chosen" state) rather than no
+ filtering at all. Shared by ``apply_dashboard_filters`` (applied values)
+ and this module (default values on create/update) so both paths agree.
+ """
+ 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 _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' has no target "
+ "column, so a value cannot be applied to it."
+ )
+
+ control_values = conf.get("controlValues") or {}
+ if control_values.get("inverseSelection"):
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' enables inverse "
+ "selection, which this tool does not support."
+ )
+ if (operator := control_values.get("operatorType", "exact")) != "exact":
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' uses matching "
+ f"operator '{operator}', which this tool does not support. "
+ "Only exact-match select filters are supported."
+ )
+ # A single-select filter renders one value; storing several would disagree
+ # with the control the moment a viewer touches it. multiSelect defaults to
+ # true, so only an explicit false restricts the selection.
+ if control_values.get("multiSelect") is False and len(values) > 1:
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' is single-select "
+ f"and accepts at most one value, but {len(values)} were given."
+ )
+
+ if values:
+ extra_form_data: dict[str, Any] = {
+ "filters": [{"col": column, "op": "IN", "val": list(values)}]
+ }
+ filter_state: dict[str, Any] = {
+ "value": list(values),
+ "label": ", ".join(_value_label(value) for value in values),
+ }
+ else:
+ extra_form_data = (
+ {
+ "adhoc_filters": [
+ {
+ "expressionType": "SQL",
+ "clause": "WHERE",
+ "sqlExpression": EMPTY_FILTER_SQL_EXPRESSION,
+ }
+ ]
+ }
+ if control_values.get("enableEmptyFilter")
+ else {}
+ )
+ filter_state = {"value": None}
+
+ return {"extraFormData": extra_form_data, "filterState": filter_state}
+
+
+def _default_data_mask(
+ conf: dict[str, Any], values: list[FilterSelectValue]
+) -> dict[str, Any]:
+ """Build the stored default data mask for a filter_select filter.
+
+ Clearing a default (empty ``values``) writes no predicate, so it must stay
+ possible on filters created in the UI with inverse selection or a
+ non-exact operator; those filters get the plain empty mask. Non-empty
+ defaults go through ``_select_data_mask`` and its guards.
+ """
+ control_values = conf.get("controlValues") or {}
+ if not values and (
+ control_values.get("inverseSelection")
+ or control_values.get("operatorType", "exact") != "exact"
+ ):
+ return _empty_data_mask()
Review Comment:
**Suggestion:** For a non-exact select with `enableEmptyFilter=True`,
clearing the default takes this branch and removes the required `1 = 0`
predicate, so queries include all rows.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Incorrect condition
logic`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=72b746e0bfe048ab9e7213fff8eff9aa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=72b746e0bfe048ab9e7213fff8eff9aa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/manage_native_filters.py
**Line:** 163:167
**Comment:**
*Incorrect Condition Logic: For a non-exact select with
`enableEmptyFilter=True`, clearing the default takes this branch and removes
the required `1 = 0` predicate, so queries include all rows.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44985&comment_hash=e35692f57cbd1ff900c4512b084bc9cbc347c403b822dccf2b224962474977f7&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44985&comment_hash=e35692f57cbd1ff900c4512b084bc9cbc347c403b822dccf2b224962474977f7&reaction=dislike'>๐</a>
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -211,6 +318,57 @@ def _merge_target(spec: NativeFilterUpdateSpec, merged:
dict[str, Any]) -> None:
merged["targets"] = [target]
+def _target_key(target: dict[str, Any]) -> tuple[Any, Any]:
+ """Identify a filter target by its dataset and column name."""
+ return target.get("datasetId"), (target.get("column") or {}).get("name")
Review Comment:
**Suggestion:** Updating only `column` on a legacy filter whose target
stores `column` as a string reaches this call and raises `AttributeError`, so
the update fails.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Type error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a13cb527f6a74bbaaa99af9fde50a673&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a13cb527f6a74bbaaa99af9fde50a673&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/manage_native_filters.py
**Line:** 323:323
**Comment:**
*Type Error: Updating only `column` on a legacy filter whose target
stores `column` as a string reaches this call and raises `AttributeError`, so
the update fails.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44985&comment_hash=d6255da424b71983ddc3e34438488aef61d1512fb51dd375c7f949f206b2a6b4&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44985&comment_hash=d6255da424b71983ddc3e34438488aef61d1512fb51dd375c7f949f206b2a6b4&reaction=dislike'>๐</a>
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -64,6 +72,102 @@ def _empty_data_mask() -> dict[str, Any]:
return {"filterState": {"value": None}, "extraFormData": {}}
+def _value_label(value: FilterSelectValue) -> str:
+ """Format one selected value the way the dashboard UI labels it."""
+ if value is None:
+ return _NULL_LABEL
+ if isinstance(value, bool):
+ return _TRUE_LABEL if value else _FALSE_LABEL
+ return str(value)
+
+
+def _select_data_mask(
+ conf: dict[str, Any], values: list[FilterSelectValue]
+) -> dict[str, Any]:
+ """Build the data mask a filter_select filter produces for ``values``.
+
+ Mirrors the frontend's ``getSelectExtraFormData``: a non-empty selection
+ becomes an ``IN`` predicate on the filter's target column, and an empty
+ selection on a filter marked ``enableEmptyFilter`` becomes an impossible
+ predicate (the "required filter, nothing chosen" state) rather than no
+ filtering at all. Shared by ``apply_dashboard_filters`` (applied values)
+ and this module (default values on create/update) so both paths agree.
+ """
+ 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 _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' has no target "
+ "column, so a value cannot be applied to it."
+ )
+
+ control_values = conf.get("controlValues") or {}
+ if control_values.get("inverseSelection"):
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' enables inverse "
+ "selection, which this tool does not support."
+ )
+ if (operator := control_values.get("operatorType", "exact")) != "exact":
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' uses matching "
+ f"operator '{operator}', which this tool does not support. "
+ "Only exact-match select filters are supported."
+ )
+ # A single-select filter renders one value; storing several would disagree
+ # with the control the moment a viewer touches it. multiSelect defaults to
+ # true, so only an explicit false restricts the selection.
+ if control_values.get("multiSelect") is False and len(values) > 1:
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' is single-select "
+ f"and accepts at most one value, but {len(values)} were given."
+ )
+
+ if values:
+ extra_form_data: dict[str, Any] = {
+ "filters": [{"col": column, "op": "IN", "val": list(values)}]
+ }
+ filter_state: dict[str, Any] = {
+ "value": list(values),
+ "label": ", ".join(_value_label(value) for value in values),
+ }
+ else:
+ extra_form_data = (
+ {
+ "adhoc_filters": [
+ {
+ "expressionType": "SQL",
+ "clause": "WHERE",
+ "sqlExpression": EMPTY_FILTER_SQL_EXPRESSION,
+ }
+ ]
+ }
+ if control_values.get("enableEmptyFilter")
+ else {}
+ )
+ filter_state = {"value": None}
Review Comment:
**Suggestion:** When an `enableEmptyFilter` default is empty, this `None`
makes server-side dashboard context treat it as unset and omit its `1 = 0`
predicate, returning unfiltered chart data.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e4e807a66f3b4920a92a8ed726dd7961&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e4e807a66f3b4920a92a8ed726dd7961&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/manage_native_filters.py
**Line:** 147:147
**Comment:**
*Api Mismatch: When an `enableEmptyFilter` default is empty, this
`None` makes server-side dashboard context treat it as unset and omit its `1 =
0` predicate, returning unfiltered chart data.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44985&comment_hash=663a6eae773aedfd21409509729c4126fb212e9320f59f2b628f33fefecebb25&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44985&comment_hash=663a6eae773aedfd21409509729c4126fb212e9320f59f2b628f33fefecebb25&reaction=dislike'>๐</a>
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -211,6 +318,57 @@ def _merge_target(spec: NativeFilterUpdateSpec, merged:
dict[str, Any]) -> None:
merged["targets"] = [target]
+def _target_key(target: dict[str, Any]) -> tuple[Any, Any]:
+ """Identify a filter target by its dataset and column name."""
+ return target.get("datasetId"), (target.get("column") or {}).get("name")
+
+
+def _stored_default_is_stale(
+ spec: NativeFilterUpdateSpec, existing: dict[str, Any], target_changed:
bool
+) -> bool:
+ """Whether an update without ``default_value`` invalidates the stored one.
+
+ A stored default goes stale when the filter is retargeted to another
+ column or dataset, when it becomes single-select while the default holds
+ several values, or when ``default_to_first_item`` is switched on (the
+ explicit default would otherwise win and the first item never applies).
+ """
+ if existing.get("filterType") != "filter_select":
+ return False
+ stored = ((existing.get("defaultDataMask") or {}).get("filterState") or
{}).get(
+ "value"
+ )
+ if stored is None:
+ return False
Review Comment:
**Suggestion:** After an empty required default stores `value=None` with a
`1 = 0` predicate, disabling `enable_empty_filter` hits this return and retains
the predicate, leaving the dashboard empty.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Stale reference`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=810df17599414d2f93d65a3448ba43ff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=810df17599414d2f93d65a3448ba43ff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/manage_native_filters.py
**Line:** 341:342
**Comment:**
*Stale Reference: After an empty required default stores `value=None`
with a `1 = 0` predicate, disabling `enable_empty_filter` hits this return and
retains the predicate, leaving the dashboard empty.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44985&comment_hash=830e09dc7611395fe1d7b871c60b2cf0655a8773ae5aead5ab0747f5acee12bf&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44985&comment_hash=830e09dc7611395fe1d7b871c60b2cf0655a8773ae5aead5ab0747f5acee12bf&reaction=dislike'>๐</a>
--
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]
