eschutho commented on PR #42284: URL: https://github.com/apache/superset/pull/42284#issuecomment-5247176313
Thanks for this — tested it live end-to-end (real Celery worker, MailHog, MinIO-backed S3) rather than just reading the diff, and found one correctness gap worth fixing before merge. **The `granularity` gate in `build_query_context_from_form_data` (`superset/common/form_data_query_context.py`) still drops time-bucketing for `table`/`pie` charts grouped by their own time column when `time_range` is `"No filter"`.** Repro (verified against a real dataset with per-day granularity, not one that happens to already be pre-aggregated to the grain being tested): - Chart: `table`, `groupby: ["order_date"]`, `granularity_sqla: "order_date"`, `time_grain_sqla: "P1Y"`, `time_range: "No filter"`, no saved `query_context`. - Exported sheet: **252 rows**, each a raw, un-truncated `order_date` timestamp (e.g. `2003-11-14`) — i.e. completely unaggregated data. - Same chart, only difference `time_range: "2003-01-01 : 2006-01-01"`: **3 rows**, correctly bucketed by year with proper aggregated counts. Root cause: `granularity` is only set when `time_range != "No filter"` (or for the `big_number` trendline's promoted time column). But `granularity` and `time_range` are independent concerns in the real query object — I checked `extractExtras.ts` in `superset-ui-core`, and the frontend sets `granularity` from `granularity_sqla`/`granularity` unconditionally, with zero dependency on `time_range`. The backend's groupby-column loop (`superset/models/helpers.py`) only applies time-grain truncation to a selected column when it matches `granularity` — so omitting it here means the column is selected raw instead of bucketed, for the fairly ordinary "all-time totals by month/year" chart configuration. The `big_number` trendline fix (`promoted_time_column`) is correct and I confirmed it works — it's specifically the general `table`/`pie` "explicit `groupby` includes the time column" case that's still affected. I've added a regression test that reproduces this (currently failing against HEAD): `test_table_groupby_time_column_without_time_range_is_bucketed` in `tests/unit_tests/common/test_form_data_query_context.py`. Suggested fix: set `granularity` whenever `granularity_sqla`/`granularity` is present, independent of `time_range` (matching real frontend behavior), rather than gating on `time_range != "No filter"`. Everything else I exercised — happy-path rebuilds for all four allowlisted viz types, the `_needs_unsupported_processing` guard, malformed-`query_context` recovery, the in-flight export lock, and the full email→S3→download round trip via a real Celery worker — worked correctly. -- 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]
