aminghadersohi commented on PR #43737: URL: https://github.com/apache/superset/pull/43737#issuecomment-5560853658
Thanks for the full review. This is a finite maintainer pass; no new reviewer run was requested and the human CHANGES_REQUESTED review has not been dismissed. Disposition of every item in review 5089944651: 1. **Mixed valid/invalid intervals — fixed.** The shared saved/unsaved Gantt Vega-Lite builder skips null, unparsable, reversed, and unlabeled intervals while retaining renderable rows. Nonempty all-invalid results still error; empty results remain valid. Missing fields/non-object rows still error, rather than hiding malformed query output. The inverted parametrized regression covers invalid start/end values and categories. 2. **Malformed native ordering — retained deliberately.** Native Gantt/buildQuery.ts uses JSON.parse for every sort entry and does not ignore malformed JSON. Dropping a sort silently also changes the rows selected under row_limit. The bounded, strict [column, ascending_boolean] contract and test_saved_query_fallback_rejects_malformed_native_order are retained; this is distinct from omitting non-renderable intervals. The user docs state this limitation. 3. **Raw preview payload size — fixed.** The returned specification contains at most 1,000 renderable rows and only the fields used by encodings/tooltips. The cap is applied after skipping invalid intervals; a regression includes >1,000 invalid rows before >1,000 valid rows and an unused payload field. 4. **Shared dataset validation exception policy — deferred as permitted.** _validate_dataset's broad exception fallback is pre-existing shared behavior on upstream master. Narrowing it needs a separate cross-chart policy change; this pass does not claim to fix it. 5. **Filter docstring — fixed.** The filter merger lives in shared chart_utils.preserve_previous_adhoc_filters and documents replacement of the prior marker-owned temporal binding. Saved updates, their previews, and cached previews reuse it. Inactive native Gantt No filter placeholders are replaced without dropping an active predicate. 6. **Example parse coverage — fixed.** test_chart_examples.py parametrizes every _CHART_EXAMPLES example through TypeAdapter(ChartConfig) (including upstream Gauge), and reads the chart-config resource with authentication isolated to validate every published example config, including project_schedule. 7. **Existing-path coordination — documented in the PR body.** Shared compile validates SQL WHERE/HAVING filters; Big Number trendline queries keep the temporal column (not big_number_total). Also restored explicit top-level orderby through the shared builder, addressing the earlier issue comment with a regression. Other issue-comment dispositions: - Explicit ordering: fixed in this pass. Shared Gantt preview duplication, Big Number temporal extraction, metric-vs-column validation, and query-result failure handling have existing focused regressions in the branch. - The frontend snake_case form controls are intentionally retained: snake-to-camel conversion happens before transformProps; changing persisted keys to camelCase would not match the control contract. - Native series arrays/single scalar tooltip metrics are outside the bounded typed adapter contract: the Gantt UI exposes one series dimension and list-valued tooltip metrics. ensureIsArray in query assembly is not evidence that the renderer supports multiple series dimensions. - Required-role casefold collision rejection is intentional and covered across request, native merge, and canonical-name paths; metadata lookup still permits exact physical names in case-colliding datasets. This pass does not broaden the supported required-role collision contract. - Metric-only ambiguous spellings without saved_metric=true remain rejected with the tailored opt-in hint; no arbitrary metric is executed. - Saved-update validation retains its established GenerateChartResponse envelope rather than changing the client-facing error discriminator solely for stylistic consistency. - Removed the unnecessary extra Gantt dataset-context build on non-Gantt cached previews, isolated datasource-engine lookup in the Big Number unit regression, and made the temporal-column fixture lookup name-based. - Duplicate sort classes, small repeated query-error constructors, and repeated local test setup are style-only; retained to avoid unrelated abstractions and validation/default changes. Evidence: normal merge f68b095f and fix commit 1614ed40238e204ee2007ec619411a4fa0e6a841. Exact-head chart suite: 1,598 passed; focused Gantt/Gauge/examples/query-helper/cached-update suite: 321 passed. Staged and full-PR-range pre-commit (MyPy, Ruff, Pylint), changed-file compilation, and diff checks passed. Required CI is being tracked on the pushed exact head; final CI/broad-suite results will be 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]
