hughhhh commented on PR #42284: URL: https://github.com/apache/superset/pull/42284#issuecomment-5061179587
Thanks — all findings addressed in `719649fe1a`, and I re-validated end-to-end against the live stack (which caught a regression the static review couldn't, see below). **🔴 Functional** - **granularity / time range** — the rebuild now sets `granularity` (from `granularity`/`granularity_sqla`) so `time_range` is actually applied. **Live validation caught an over-reach here:** the example charts store `granularity_sqla: "year"` (a numeric column) with `time_range: "No filter"`, and unconditionally setting `granularity` made the query `date_trunc` a numeric column → charts that exported fine now failed. Fixed by only setting `granularity` when there's an *active* range (which is exactly the reviewer's scenario). Confirmed: the 3 charts export again, and no `date_trunc` error. - **custom-SQL filters** — `SQL` adhoc filters + legacy top-level `where` now map into `extras.where`/`extras.having` by clause (mirroring `processFilters`), instead of being dropped. - **orderby** — derived from `order_by_cols` (raw) or `timeseries_limit_metric`/`sort_by_metric`/`order_desc`, falling back to first-metric-descending. **Live-confirmed:** the "Games" table now exports "Wii Sports" (82.7) first instead of an arbitrary "Happy Feet Two" (0.1). - **table percent_metrics / time_grain** — `percent_metrics` carried into metrics; `time_grain_sqla` passed via `extras`. - **big_number_total** — granularity_sqla is promoted to a grouping column only for the trendline viz (`big_number`), never `big_number_total`. **🟡 Should-fix** - **duplication** — MCP `adhoc_filters_to_query_filters` (chart_utils) and `_build_query_columns` (preview_utils) now delegate to the shared module, so the `columns` bug divergence is gone; MCP tests still pass. - **test depth** — added an export test asserting the full query body (columns/filters/orderby/granularity/time_range/row_limit) that reaches `ChartDataQueryContextSchema().load`, plus unit tests for every derived field. **🔵 Nits** — restructured the loop to drop the `-O`-stripped `assert`, parse the saved `query_context` once, and corrected the shallow-copy comment. `form_data_query_context.py` is at 100% coverage; all unit + affected MCP tests green. -- 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]
