aminghadersohi commented on PR #45242: URL: https://github.com/apache/superset/pull/45242#issuecomment-6145719367
Reviewed the complete diff against master at `44ab996c7aae9c50a5ff9d3f88c90223a0e8ea5a` for https://github.com/apache/superset/pull/45242. Fixes are in `20ba9678d14d0459c60193087915dff178f50304`. Findings closed: - **Medium — truncated search falsely established absence.** An exact value can be hidden beyond both bounded pages. Such an inconclusive negative lookup preserves the requested value; a partial normalized match still fails with an explicit uncertainty message instead of guessing. - **Medium — search and normalization used incompatible representations.** SQL `lower()` does not implement Python Unicode casefolding, and native/date column text casts need not match JSON labels. Search terms exclude casefold-expansion fragments; unsupported casts/non-ASCII lookups remain unchecked rather than falsely rejecting valid values. Real SQLite regressions cover sharp-s, long-s, ligatures, Kelvin sign, and whitespace. - **Medium — driver/structured values could break matching or the response schema.** Column options are converted to their JSON scalar representation before matching. Backend dates now resolve safely; unsupported whole-array options use the existing unchecked fallback rather than crashing. - **Medium — ambiguity errors told the caller the value was absent and hid colliding alternatives.** Ambiguity and incomplete normalized searches have separate messages with typed exact alternatives. Fuzzy suggestions retain distinct case/type collisions instead of collapsing them. - **Low — duplicate selections repeated expensive lookups.** Requested values are deduplicated before validation and search results are reused within the invocation, including case variants and empty search terms. The repeated-selection regression requires only the initial query and one search. - **Low — misleading recovery guidance.** NULL-only columns now offer `null` rather than claiming the column is empty. Mixed valid/missing selections no longer claim they necessarily empty every chart. Permission/scoping review: dashboard access precedes dataset lookup; dataset access precedes value reads; initial queries and searches retain the existing effective RLS predicates. Real-query regressions cover normal and guest RLS without exposing restricted suggestions, and denied access never queries values. No high/critical or permission-boundary finding was identified. Row-cap, column-normalization, suggestion-bound, and atomic-failure coverage is retained/extended. Documentation explains that unchecked fallback is not certification of presence; only this tool's inventory entry and associated aggregate sizes were updated. Validation: - `pytest -q -n 4 tests/unit_tests/mcp_service/`: **10,758 passed, 11 skipped**. - Apply-filter tests: **178 passed**, including **24 additional cases**. - Ruff checks/formatting, pylint (10.00/10), and pre-commit across every branch-changed file, including mypy: **passed**. -- 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]
