codeant-ai-for-open-source[bot] commented on code in PR #45242:
URL: https://github.com/apache/superset/pull/45242#discussion_r4257398569
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -158,6 +172,208 @@ def _filter_target_column(conf: dict[str, Any]) -> str:
return column
+def _target_dataset(conf: dict[str, Any]) -> "SqlaTable | None":
+ """Return the dataset a filter's first target points at, if it resolves."""
+ 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
+ return DatasetDAO.find_by_id(dataset_id) if dataset_id is not None else
None
+
+
+def _column_values(
+ dataset: "SqlaTable", column: str, search: str | None = None
+) -> tuple[list[Any], bool]:
+ """Return ``column``'s distinct values and whether that list is complete.
+
+ Uses the same bounded, RLS-aware query that feeds the dashboard's select
+ filter options, so the values checked here are the ones a viewer could
+ pick. ``search`` narrows it to values containing the term,
+ case-insensitively.
+ """
+ limit = apply_max_row_limit(current_app.config["FILTER_SELECT_ROW_LIMIT"])
+ values = dataset.values_for_column(
+ column_name=column,
+ limit=limit,
+ denormalize_column=not dataset.normalize_columns,
+ search=search,
+ )
+ return values, len(values) < limit
+
+
+def _normalize_value(value: FilterSelectValue) -> str:
+ """Fold a value to the text it is compared by when not matched exactly."""
+ return " ".join(_value_label(value).split()).casefold()
+
+
+def _match_value(value: FilterSelectValue, candidates: Sequence[Any]) -> Any:
+ """Return the column value ``value`` refers to, or ``_NOT_FOUND``.
+
+ An exact match keeps the requested value. Otherwise a single column
+ value whose text matches ignoring case and whitespace (``"emea"`` for
+ ``"EMEA"``, ``"2024"`` for ``2024``) is used instead, since filtering on
+ the requested spelling would match no rows. Several such values are
+ ambiguous and count as no match.
+ """
+ for candidate in candidates:
+ # bool is an int subclass, so True would otherwise match 1.
+ if candidate == value and isinstance(candidate, bool) == isinstance(
+ value, bool
+ ):
+ return value
+ if value is None:
+ return _NOT_FOUND
+ wanted = _normalize_value(value)
+ matches = {
+ _value_label(candidate): candidate
+ for candidate in candidates
+ if candidate is not None and _normalize_value(candidate) == wanted
+ }
Review Comment:
**Suggestion:** Numeric `1` and string `'1'` candidates share this label, so
the dict collapses two normalized matches and may apply one arbitrarily instead
of rejecting the ambiguity.
**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=1b744667d98f41cd994d20d601d07192&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=1b744667d98f41cd994d20d601d07192&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/apply_dashboard_filters.py
**Line:** 227:231
**Comment:**
*Incorrect Condition Logic: Numeric `1` and string `'1'` candidates
share this label, so the dict collapses two normalized matches and may apply
one arbitrarily instead of rejecting the ambiguity.
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%2F45242&comment_hash=4716a438099c95e25d53fbbf297ae4006245e11f114aa9744f4f4be68bf1ee47&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45242&comment_hash=4716a438099c95e25d53fbbf297ae4006245e11f114aa9744f4f4be68bf1ee47&reaction=dislike'>๐</a>
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -158,6 +172,208 @@ def _filter_target_column(conf: dict[str, Any]) -> str:
return column
+def _target_dataset(conf: dict[str, Any]) -> "SqlaTable | None":
+ """Return the dataset a filter's first target points at, if it resolves."""
+ 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
+ return DatasetDAO.find_by_id(dataset_id) if dataset_id is not None else
None
+
+
+def _column_values(
+ dataset: "SqlaTable", column: str, search: str | None = None
+) -> tuple[list[Any], bool]:
+ """Return ``column``'s distinct values and whether that list is complete.
+
+ Uses the same bounded, RLS-aware query that feeds the dashboard's select
+ filter options, so the values checked here are the ones a viewer could
+ pick. ``search`` narrows it to values containing the term,
+ case-insensitively.
+ """
+ limit = apply_max_row_limit(current_app.config["FILTER_SELECT_ROW_LIMIT"])
+ values = dataset.values_for_column(
+ column_name=column,
+ limit=limit,
+ denormalize_column=not dataset.normalize_columns,
+ search=search,
+ )
+ return values, len(values) < limit
+
+
+def _normalize_value(value: FilterSelectValue) -> str:
+ """Fold a value to the text it is compared by when not matched exactly."""
+ return " ".join(_value_label(value).split()).casefold()
+
+
+def _match_value(value: FilterSelectValue, candidates: Sequence[Any]) -> Any:
+ """Return the column value ``value`` refers to, or ``_NOT_FOUND``.
+
+ An exact match keeps the requested value. Otherwise a single column
+ value whose text matches ignoring case and whitespace (``"emea"`` for
+ ``"EMEA"``, ``"2024"`` for ``2024``) is used instead, since filtering on
+ the requested spelling would match no rows. Several such values are
+ ambiguous and count as no match.
+ """
+ for candidate in candidates:
+ # bool is an int subclass, so True would otherwise match 1.
+ if candidate == value and isinstance(candidate, bool) == isinstance(
+ value, bool
+ ):
+ return value
+ if value is None:
+ return _NOT_FOUND
+ wanted = _normalize_value(value)
+ matches = {
+ _value_label(candidate): candidate
+ for candidate in candidates
+ if candidate is not None and _normalize_value(candidate) == wanted
+ }
+ return next(iter(matches.values())) if len(matches) == 1 else _NOT_FOUND
+
+
+def _suggest_values(value: FilterSelectValue, candidates: Sequence[Any]) ->
list[Any]:
+ """Return column values that look like what ``value`` was meant to be."""
+ wanted = _normalize_value(value)
+ by_label: dict[str, Any] = {}
+ for candidate in candidates:
+ if candidate is not None:
+ by_label.setdefault(_normalize_value(candidate), candidate)
+ suggested = [label for label in by_label if wanted and wanted in label]
+ suggested += difflib.get_close_matches(
+ wanted, list(by_label), n=_MAX_VALUE_SUGGESTIONS, cutoff=0.6
+ )
+ return [by_label[label] for label in dict.fromkeys(suggested)][
+ :_MAX_VALUE_SUGGESTIONS
+ ]
+
+
+def _quote_values(values: Sequence[Any]) -> str:
+ """Render values as a quoted, comma-separated list."""
+ return ", ".join(f"'{_value_label(value)}'" for value in values)
+
+
+def _unknown_values_error(
+ conf: dict[str, Any],
+ column: str,
+ unknown: list[tuple[FilterSelectValue, list[Any]]],
+ candidates: list[Any],
+ complete: bool,
+) -> _FilterApplyError:
+ """Build the error for requested values that are absent from the data."""
+ name = conf.get("name") or conf.get("id")
+ noun = "value" if len(unknown) == 1 else "values"
+ message = [
+ f"Filter '{name}' has no {noun} {_quote_values([v for v, _ in
unknown])} "
+ f"in column '{column}', so applying it would leave every chart it "
+ "filters empty. No filter values were applied."
+ ]
+ for value, suggestions in unknown:
+ if suggestions:
+ message.append(
+ f"Closest matches for '{_value_label(value)}': "
+ f"{_quote_values(suggestions)}."
+ )
+ if not any(suggestions for _, suggestions in unknown):
+ shown = sorted(
+ (value for value in candidates if value is not None),
key=_value_label
+ )[:_MAX_VALUE_SUGGESTIONS]
+ if not shown:
+ message.append(
+ "The column has no values to filter on. Tell the user the "
+ "value was not found instead of reporting the filter as
applied."
+ )
+ return _FilterApplyError(" ".join(message))
+ total = f" ({len(candidates)} in total)" if complete else ""
+ prefix = "Available values" if complete else "Available values include"
+ message.append(f"{prefix}: {_quote_values(shown)}{total}.")
+ message.append(
+ "Tell the user the value was not found and offer these values instead "
+ "of reporting the filter as applied."
+ )
+ return _FilterApplyError(" ".join(message))
+
+
+def _search_value(
+ dataset: "SqlaTable",
+ conf: dict[str, Any],
+ column: str,
+ value: FilterSelectValue,
+) -> tuple[Any, list[Any]]:
+ """Look ``value`` up across the whole column, past a truncated first page.
+
+ Returns the matched value (or ``_NOT_FOUND``) and the values the search
+ returned, for suggestions. A value the search cannot settle (NULL, which
+ a containment search cannot find, or a failed query) is kept as given:
+ absence from a truncated page proves nothing.
+ """
+ if value is None:
+ return value, []
+ try:
+ searched, _ = _column_values(dataset, column,
search=_value_label(value))
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning(
+ "Could not search column %s for filter %s",
+ column,
+ conf.get("id"),
+ exc_info=True,
+ )
+ return value, []
+ return _match_value(value, searched), searched
+
+
+def _verify_select_values(
+ conf: dict[str, Any], column: str, values: list[FilterSelectValue]
+) -> list[FilterSelectValue]:
+ """Check requested select values against the filter column's data.
+
+ Returns the values to apply, with near matches replaced by the column
+ value they match, and raises when a value matches nothing: an ``IN``
+ predicate on a value the column lacks would silently empty every chart
+ in the filter's scope. When the values cannot be read (unresolvable or
+ inaccessible dataset, query failure, unsupported datasource) they are
+ applied unchecked, as before, rather than blocking the request.
+ """
+ from superset.exceptions import SupersetSecurityException
+
+ try:
+ dataset = _target_dataset(conf)
+ if dataset is None:
+ return values
+ # The column-values API gates on the same check before reading values.
+ dataset.raise_for_access()
+ candidates, complete = _column_values(dataset, column)
+ except SupersetSecurityException:
+ return values
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning(
+ "Could not read values of column %s for filter %s; applying the "
+ "requested values unchecked",
+ column,
+ conf.get("id"),
+ exc_info=True,
+ )
+ return values
+
+ resolved: list[FilterSelectValue] = []
+ # Keyed by type too: True == 1, but they are different selections.
+ seen: set[tuple[type, FilterSelectValue]] = set()
+ unknown: list[tuple[FilterSelectValue, list[Any]]] = []
+ for value in values:
+ match = _match_value(value, candidates)
+ searched: list[Any] = []
+ if match is _NOT_FOUND and not complete:
+ match, searched = _search_value(dataset, conf, column, value)
Review Comment:
**Suggestion:** When a truncated first page contains one case- or
spacing-insensitive match, this skips the full-column search; another match
beyond the page is missed and the wrong value is applied.
**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=af32ca3463394ecb98f1659e68bf73c6&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=af32ca3463394ecb98f1659e68bf73c6&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/apply_dashboard_filters.py
**Line:** 365:366
**Comment:**
*Incorrect Condition Logic: When a truncated first page contains one
case- or spacing-insensitive match, this skips the full-column search; another
match beyond the page is missed and the wrong value is applied.
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%2F45242&comment_hash=5902196f6932e0eb6d525a6f9a5008e0b46594ae525a3b5e3e1db3043bf8621b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45242&comment_hash=5902196f6932e0eb6d525a6f9a5008e0b46594ae525a3b5e3e1db3043bf8621b&reaction=dislike'>๐</a>
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -158,6 +172,208 @@ def _filter_target_column(conf: dict[str, Any]) -> str:
return column
+def _target_dataset(conf: dict[str, Any]) -> "SqlaTable | None":
+ """Return the dataset a filter's first target points at, if it resolves."""
+ 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
+ return DatasetDAO.find_by_id(dataset_id) if dataset_id is not None else
None
+
+
+def _column_values(
+ dataset: "SqlaTable", column: str, search: str | None = None
+) -> tuple[list[Any], bool]:
+ """Return ``column``'s distinct values and whether that list is complete.
+
+ Uses the same bounded, RLS-aware query that feeds the dashboard's select
+ filter options, so the values checked here are the ones a viewer could
+ pick. ``search`` narrows it to values containing the term,
+ case-insensitively.
+ """
+ limit = apply_max_row_limit(current_app.config["FILTER_SELECT_ROW_LIMIT"])
+ values = dataset.values_for_column(
+ column_name=column,
+ limit=limit,
+ denormalize_column=not dataset.normalize_columns,
+ search=search,
+ )
+ return values, len(values) < limit
+
+
+def _normalize_value(value: FilterSelectValue) -> str:
+ """Fold a value to the text it is compared by when not matched exactly."""
+ return " ".join(_value_label(value).split()).casefold()
+
+
+def _match_value(value: FilterSelectValue, candidates: Sequence[Any]) -> Any:
+ """Return the column value ``value`` refers to, or ``_NOT_FOUND``.
+
+ An exact match keeps the requested value. Otherwise a single column
+ value whose text matches ignoring case and whitespace (``"emea"`` for
+ ``"EMEA"``, ``"2024"`` for ``2024``) is used instead, since filtering on
+ the requested spelling would match no rows. Several such values are
+ ambiguous and count as no match.
+ """
+ for candidate in candidates:
+ # bool is an int subclass, so True would otherwise match 1.
+ if candidate == value and isinstance(candidate, bool) == isinstance(
+ value, bool
+ ):
+ return value
+ if value is None:
+ return _NOT_FOUND
+ wanted = _normalize_value(value)
+ matches = {
+ _value_label(candidate): candidate
+ for candidate in candidates
+ if candidate is not None and _normalize_value(candidate) == wanted
+ }
+ return next(iter(matches.values())) if len(matches) == 1 else _NOT_FOUND
+
+
+def _suggest_values(value: FilterSelectValue, candidates: Sequence[Any]) ->
list[Any]:
+ """Return column values that look like what ``value`` was meant to be."""
+ wanted = _normalize_value(value)
+ by_label: dict[str, Any] = {}
+ for candidate in candidates:
+ if candidate is not None:
+ by_label.setdefault(_normalize_value(candidate), candidate)
+ suggested = [label for label in by_label if wanted and wanted in label]
+ suggested += difflib.get_close_matches(
+ wanted, list(by_label), n=_MAX_VALUE_SUGGESTIONS, cutoff=0.6
+ )
+ return [by_label[label] for label in dict.fromkeys(suggested)][
+ :_MAX_VALUE_SUGGESTIONS
+ ]
+
+
+def _quote_values(values: Sequence[Any]) -> str:
+ """Render values as a quoted, comma-separated list."""
+ return ", ".join(f"'{_value_label(value)}'" for value in values)
+
+
+def _unknown_values_error(
+ conf: dict[str, Any],
+ column: str,
+ unknown: list[tuple[FilterSelectValue, list[Any]]],
+ candidates: list[Any],
+ complete: bool,
+) -> _FilterApplyError:
+ """Build the error for requested values that are absent from the data."""
+ name = conf.get("name") or conf.get("id")
+ noun = "value" if len(unknown) == 1 else "values"
+ message = [
+ f"Filter '{name}' has no {noun} {_quote_values([v for v, _ in
unknown])} "
+ f"in column '{column}', so applying it would leave every chart it "
+ "filters empty. No filter values were applied."
+ ]
+ for value, suggestions in unknown:
+ if suggestions:
+ message.append(
+ f"Closest matches for '{_value_label(value)}': "
+ f"{_quote_values(suggestions)}."
+ )
+ if not any(suggestions for _, suggestions in unknown):
+ shown = sorted(
+ (value for value in candidates if value is not None),
key=_value_label
+ )[:_MAX_VALUE_SUGGESTIONS]
+ if not shown:
+ message.append(
+ "The column has no values to filter on. Tell the user the "
+ "value was not found instead of reporting the filter as
applied."
+ )
+ return _FilterApplyError(" ".join(message))
+ total = f" ({len(candidates)} in total)" if complete else ""
+ prefix = "Available values" if complete else "Available values include"
+ message.append(f"{prefix}: {_quote_values(shown)}{total}.")
+ message.append(
+ "Tell the user the value was not found and offer these values instead "
+ "of reporting the filter as applied."
+ )
+ return _FilterApplyError(" ".join(message))
+
+
+def _search_value(
+ dataset: "SqlaTable",
+ conf: dict[str, Any],
+ column: str,
+ value: FilterSelectValue,
+) -> tuple[Any, list[Any]]:
+ """Look ``value`` up across the whole column, past a truncated first page.
+
+ Returns the matched value (or ``_NOT_FOUND``) and the values the search
+ returned, for suggestions. A value the search cannot settle (NULL, which
+ a containment search cannot find, or a failed query) is kept as given:
+ absence from a truncated page proves nothing.
+ """
+ if value is None:
+ return value, []
+ try:
+ searched, _ = _column_values(dataset, column,
search=_value_label(value))
Review Comment:
**Suggestion:** On a truncated column, extra or repeated spaces in the
request are passed unchanged to the search, so the matching stored value can be
missed and falsely rejected.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3f5333d925f0447bb48d2cd4e64b7317&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=3f5333d925f0447bb48d2cd4e64b7317&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/apply_dashboard_filters.py
**Line:** 313:313
**Comment:**
*Logic Error: On a truncated column, extra or repeated spaces in the
request are passed unchanged to the search, so the matching stored value can be
missed and falsely rejected.
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%2F45242&comment_hash=bb1ce300c28dded948ef048b5418077eb9e47416a7529fddc353c483622b653b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45242&comment_hash=bb1ce300c28dded948ef048b5418077eb9e47416a7529fddc353c483622b653b&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]
