aminghadersohi commented on PR #45598: URL: https://github.com/apache/superset/pull/45598#issuecomment-6345789382
@rebenitez1802 I pushed 81df187b05fcfe51bf40ac3bea4a46a6213c8e51 with the following changes for the body-only review: 1. **Epoch equality values:** fixed. Sunburst equality filters retain raw epochs; Date values serialize as milliseconds. The shared temporal helper also returns numeric equality values, including its Timeseries callers. Regression: `ungrained drill to detail keeps the epoch value for native engine coercion` in `superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/EchartsSunburst.test.tsx`. 2. **Drill design / Hours offset:** both Sunburst drill actions use raw `==` plus `grain`, without constructing and discarding a range. The backend applies dataset offset before truncation for grained comparisons on engines supporting SQL temporal shifts, including physical columns, inline adhoc columns, and adhoc labels. Regressions: `test_grained_drill_equality_returns_the_segment_rows_with_hours_offset` and `test_grained_drill_filter_by_adhoc_label_applies_hours_offset` in `tests/unit_tests/models/test_hours_offset_bound_truncation.py`; these check actual row identities/counts with positive and negative offsets. Remaining: stacking with https://github.com/apache/superset/pull/45571 and adopting `getSunburstTimeGrain` in its `buildQuery` are not complete. I sent the sibling worker this head and the merge-first/shared-helper plan; I did not modify that branch. 3. **Effective Time Column:** fixed. Dashboard granularity takes precedence; implicit semantic graining requires exactly one selected temporal level. Regressions in `superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/utils.test.ts`: `does not grain ambiguous semantic hierarchies with two temporal levels` and `dashboard time column takes precedence in a two-temporal-level hierarchy`. 4. **Unknown grains:** fixed in the shared fallback: numeric `==` plus `grain`, with the regular label formatter. The six engine/addon grain cases in Sunburst test `preserves the engine grain %s for cross-filter and drill equality` cover both drill actions and cross-filtering. Timeseries caller regression: `temporal drill by preserves native epoch coercion and fallback grain %s` in `superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/EchartsTimeseries.test.tsx`. 5. **Drill by time-range overwrite:** fixed for Sunburst using the same equality-plus-grain helper as Drill to detail. Regression: `drill by preserves a grained bucket when the chart has a time range`. Remaining: the pre-existing Timeseries known-grain range behavior is unchanged and documented in the description. 6. **Label scope:** restricted the adaptive override to Month/Quarter/Year; Week/Day/Hour and unknown grains retain their compact formatter. Regression: `keeps compact smart_date labels for %s` in `superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts`. I updated the description and docs. 7. **Selection keys:** Date keys use `getTime()`; unchanged legacy display-label keys remain selectable/highlighted. Regressions: `Date selection keys use epoch milliseconds` and `legacy label-based temporal selections remain highlighted and click to clear`. The compatibility fallback matches unchanged labels, not old month/quarter labels that were reformatted. 8. **Cross-filter Time Range side effects:** documented that bucket ranges replace the target Time Range and affect its other temporal range filters, including other time columns. 9. **Docs placement:** moved the paragraph to a dedicated Sunburst cross-filtering section and removed the upper-bound implementation detail. 10. **Test gaps / helper clarity:** added the cases above, Quarter coverage, and the Hours-offset execution regressions. The Year label case remains a preservation check, not evidence for the label change. The drill helper constructs equality directly. Fail-before/pass-after proof: with source temporarily restored to reviewed head `37e5c78a8617b5ca52b73e172ba12673f03f2e93`, the final Jest run had **26 failures** and the focused Hours-offset run had **7 failures**. After restoring the implementation: **84 Jest tests passed (5 suites)** and **432 Docker Python tests passed** across the offset, helpers, and SQLA-model suites. Scoped pre-commit passed, including TypeScript, mypy, ruff, and pylint. Browser acceptance was not rerun. Siblings checked: - superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/utils.ts — fixed in 81df187b05fcfe51bf40ac3bea4a46a6213c8e51 - superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx — fixed in 81df187b05fcfe51bf40ac3bea4a46a6213c8e51 - superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts — fixed in 81df187b05fcfe51bf40ac3bea4a46a6213c8e51 - superset-frontend/plugins/plugin-chart-echarts/src/utils/xAxisDrillByFilter.ts — fixed in 81df187b05fcfe51bf40ac3bea4a46a6213c8e51 - superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:685 — fixed in 81df187b05fcfe51bf40ac3bea4a46a6213c8e51 - superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:702 — fixed in 81df187b05fcfe51bf40ac3bea4a46a6213c8e51 - superset/models/helpers.py:5343 — fixed in 81df187b05fcfe51bf40ac3bea4a46a6213c8e51 - superset/models/helpers.py:5382 — fixed in 81df187b05fcfe51bf40ac3bea4a46a6213c8e51 - superset/models/helpers.py:5416 — fixed in 81df187b05fcfe51bf40ac3bea4a46a6213c8e51 - superset/common/query_context_factory.py:448 — not affected: only rewrites TEMPORAL_RANGE, while both Sunburst drill actions emit equality - superset/connectors/sqla/models.py:1322 — not affected: existing SQL-shift capability/timezone guards remain intact, and temporal range bounds retain their separate offset handling - superset/semantic_layers/mapper.py:1280 — not affected: the existing single-temporal-axis rule is mirrored by the frontend helper - superset/charts/data/api.py:228 — not affected: delegates canonical QueryObjects to the shared datasource path exercised by the Python execution regressions - superset/mcp_service/chart/chart_helpers.py:1312 — not affected: uses QueryContextFactory rather than a separate Sunburst click-filter serializer - superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/buildQuery.ts — not affected: this head has no custom grain-detection branch; sibling adoption remains listed above -- 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]
