aminghadersohi commented on PR #41895: URL: https://github.com/apache/superset/pull/41895#issuecomment-4931873558
Thanks for the thorough dual review, @fitzee — really appreciate the "door classification" framing and the concrete repro steps. Pushed `da4f9dc4e5` addressing 3 of the 4 nits: - **MEDIUM (untested guard-suppression branch):** Added `test_total_non_temporal_column_skips_temporal_filter`, mirroring `map_xy_config`'s `test_non_temporal_x_axis_no_temporal_filter`. It mocks `is_column_truly_temporal` to return `False` for an explicit `temporal_column` on `big_number_total`, so the "found a non-temporal column → suppress" branch is now covered (the existing test only hit the "column not found → default True" branch). - **LOW (duplicate id/uuid helper):** `_find_dataset_by_id_or_uuid` now delegates to `DatasetDAO.find_by_id_or_uuid` (same method the dataset API already uses) instead of reimplementing the `isdigit()` dispatch. - **LOW (`map_xy_config` double lookup):** `_resolve_default_x_axis` now returns the dataset it fetched while resolving the default x-axis, and `map_xy_config` passes it into `is_column_truly_temporal(..., dataset=...)` — same reuse pattern as `map_big_number_config`. Added a regression test asserting a single `DatasetDAO.find_by_id_or_uuid` call. **Not fixed here — LOW (trendline path: `to_form_data` + `post_map_validate` each call `is_column_truly_temporal`):** left as-is. Sharing that lookup would mean passing a resolved dataset across two separate plugin lifecycle methods (`to_form_data` then `post_map_validate`, invoked independently by `map_config_to_form_data`), which touches the shared plugin interface used by all 7 chart types rather than a local fix. Agreed it's worth doing, but I'd rather scope that as its own follow-up than fold an interface change into this bug fix. Filed for later — happy to pick it up separately if that works. All 2599 mcp_service unit tests pass, `ruff format`/`ruff check` clean. -- 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]
