aminghadersohi commented on PR #45571: URL: https://github.com/apache/superset/pull/45571#issuecomment-6347385705
@rebenitez1802 Follow-up to https://github.com/apache/superset/pull/45571#issuecomment-6346727544 Remaining: the two `getSunburstTimeGrain` helpers disagree for a hierarchy with two temporal levels and no explicit Time Column. This PR's shared `Sunburst/timeGrain.ts` grains every physical temporal level; `Sunburst/utils.ts` in https://github.com/apache/superset/pull/45598 only infers a grained level when exactly one temporal level exists, so it grains neither in that two-level case. The landing plan is for this PR to land first; https://github.com/apache/superset/pull/45598 then merges master and adopts this PR's `timeGrain.ts`. The overlapping-file conflicts and helper reconciliation remain for that separate worker; I did not edit that branch. For the broader factory change, I retained the chart-independent behavior and added regression coverage in d4351c65699f3fc187b01452854f440e61d33bd1. `_process_query_object` calls `_apply_granularity` without a chart-type condition. Table and ECharts time-series charts reach it through chart-data; samples/Drill to detail, saved-context loads, async query reconstruction, tabular queries, and MCP use the same factory. A Time Column/time-range override replaces the default `TEMPORAL_RANGE`, not an independent equality or membership predicate on that column. Removing those predicates changes the selected rows, not just the query representation. Evidence: `test_non_sunburst_granularity_preserves_independent_filters` executes SQL for Table's temporal-range fallback and time-series' temporal x-axis branch. In both, equality returns `{a: 1}`, membership `{a: 1, b: 1}`, monthly equality `{a: 2, b: 2}`, and an equality outside the time window returns `{}`. Reverting only the factory's operator guard in memory returns `{a: 2, b: 2, c: 1}` in all eight cases: **8 fail before / 8 pass after**. I also extended `test_sunburst_factory_drill_preserves_bucket_with_time_range` to the actual `drill_detail` result type; its two explicit-granularity cases fail with that guard removed and pass with it present. The two cases without granularity pass either way, as expected. Documentation and the MCP scope helper's stale description reflect the retained behavior; its conservative refusal policy is unchanged. Siblings checked: - superset/common/query_context_factory.py — fixed in 80118e754d; non-Sunburst coverage in d4351c65699f3fc187b01452854f440e61d33bd1. - superset/views/datasource/utils.py — not affected: Drill to detail and count queries delegate to the same factory; the expanded result-type matrix pins filter retention. - superset/charts/schemas.py — not affected: chart-data deserialization delegates to the same factory without a second filter-deletion rule. - superset/models/slice.py — not affected: saved contexts delegate to the same factory. - superset/common/query_serialization.py — not affected: async reconstruction delegates to the same factory; serialization tests pass. - superset/common/tabular_query.py — not affected: tabular execution delegates to the same factory; its tests pass. - superset/mcp_service/chart/chart_helpers.py — not affected: form-data construction delegates to the same factory; Sunburst/MCP tests pass. - superset/mcp_service/chart/tool/get_chart_data.py — not affected: saved-context and fallback queries use the same factory contract. - superset/mcp_service/dashboard_scope.py — not affected: its stricter refusal remains unchanged; only comments/docstring were corrected, and dashboard-scope tests pass. - superset/semantic_layers/mapper.py — not affected: explicit grain selection and main/group-limit filter mapping retain their existing regression coverage. Validation at this head: Docker Python 3.11 runs using `pytest -n 2 --dist loadfile --cov=superset` with the CI test environment passed **1,407 tests**, plus **193 tests** for dashboard scope and the guest regression (the guest test is included in both runs). Sunburst/drill/adapter Jest suites passed **128 tests in 14 suites**. Changed-file `pre-commit run --files ...` passed with `PATH=/tmp/venv45484/bin:$PATH`, whose `ruff --version` reports **0.9.7**, matching `requirements/development.txt`. All Python runs deselected `manual` and `llm_compliance`; no real-model calls were used. `docker rm -fv pr45571-python` completed; `docker ps -a --filter name=pr45571` returned no containers. -- 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]
