aminghadersohi commented on PR #43943: URL: https://github.com/apache/superset/pull/43943#issuecomment-5574824746
Finite maintainer pass on the 22 additional suggestions in [Bito's comment](https://github.com/apache/superset/pull/43943#issuecomment-5562295583), starting from `14c1d52`. Read all 6 reviews, 4 resolved threads (8 inline comments), and 3 issue comments. No new review is requested. | # | Suggestion | Disposition | |---|---|---| | 1 | Datasource override ignored | Fixed: override combined and separate datasource id/type together, without mutating stored params; regression checks the shared resolver. | | 2 | All-zero auto-range | Fixed: widen only a fully automatic degenerate range to 0–1; explicit invalid bounds still reject. | | 3 | Pointer rule not rendered | Fixed: Cartesian rule endpoints derived from the dial angle. Also give faceted units valid dimensions. Rendered 37 product-generated specs and checked finite 52px needles, visibility, angles, and facets. | | 4 | Preview function complexity | Style-only: existing complexity exemptions are intentional; configured branch-wide static checks pass. No behavior defect identified. | | 5 | Preview validation assertions | Fixed: direct Gauge renderers always validate as Gauge and return structured errors, including under Python optimization. | | 6 | Dataset rebind leaks filters | Fixed: old-dataset filters are never prepended on rebind; same-dataset temporal changes still preserve user ranges. Saved-update and preview payload regressions cover explicit clear/new binding. | | 7 | Saved metric verbose label mismatch | Stale for this path: `QueryContextProcessor.get_data` applies verbose names only to table-like exports, not JSON. Added real JSON serialization/validation regression. Mapping to a verbose name here would break valid results. | | 8 | Gauge dispatch/validation mismatch | Fixed: saved chart identity is resolved before query construction and reused for ASCII/table/Vega validation and dispatch; public saved get-data also validates with that identity. Missing/stale params discriminators are covered. | | 9 | Engine-truncated SQL result label | Stale: `make_sqla_column_compatible` preserves the expected key; `query` restores `labels_expected` on returned DataFrames. Added query-path regression with a truncated driver alias and long SQL metric label. | | 10 | Arbitrary label length caps | Fixed: remove the unrelated 20-key/2,000-character caps; retain shape checks. Long expressions and metadata-rich metrics are covered. | | 11 | Silent temporal-filter drop | Fixed: reject distinct/conflicting native temporal restrictions, including conflicts with top-level controls, instead of silently discarding one. Public FastMCP rejection happens before compilation. | | 12 | Lossless-filter claim/operators | Fixed: narrow the claim and reject unsupported native operators with an indexed, explicit error derived from FilterConfig's supported operators. | | 13 | Groupby assertion guard | Hardened with explicit ValueError; normal ColumnRef validation already rejects nameless dimensions. | | 14 | Native adapter branch count | Style-only: no incorrect behavior follows from the branch count; configured checks pass. | | 15 | Any in before-validator | Rejected: a Pydantic before-validator must accept arbitrary raw input; the existing non-dict passthrough is deliberate. A dict-only annotation would misstate its contract. | | 16 | Preserved HAVING ambiguity escapes | Fixed: return the structured ambiguity error; apply the same exact-first/unique-case-insensitive contract to preserved WHERE filters. Compiler regressions cover both clauses and exact matches. | | 17 | Aggregation ambiguity escapes | Fixed: append structured errors to the aggregation validator's existing error-list return contract; direct-call regression added. | | 18 | Only first ambiguous reference reported | Optional UX/style: the tier-1 API deliberately returns one actionable error, consistently with sibling validators. No acceptance or ambiguity-resolution inconsistency. | | 19 | Duplicate Gauge alias mapping | Style-only: all three mappings agree; existing discovery/dispatch regressions pass. No alias behavior defect. | | 20 | Private dataset-context accessor | Style-only: this internal plugin uses the existing validator helper with its supported signature and handles unavailable context. No incorrect behavior demonstrated. | | 21 | Unused module-level patch import | Stale: the public saved-Gauge FastMCP test uses the module-level import, without a local re-import. | | 22 | Unused module-level Client import | Stale: the same public FastMCP test uses the module-level Client. | The four already-resolved inline fixes are retained and covered by the broad regression run. Validation and exact-head CI are recorded in the PR body. -- 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]
