aminghadersohi commented on PR #45598: URL: https://github.com/apache/superset/pull/45598#issuecomment-6348210986
@rebenitez1802 Follow-up to review https://github.com/apache/superset/pull/45598#pullrequestreview-5618060738, at head 62cef72cee285fa71d1b5d43fe44a69ea48422ff. "Fail before" below means the test fails with the source files from 9e3574536b2b4f83ab439083f12010a94507522b (the reviewed head plus one test-only commit) and passes at the fixing commit. **🔴 Drill by drops `grain`** — fixed in 4e8c60e92ebbcec7bcc4e296677b80691361090d. I took your first fix: Sunburst Drill by sends a grained segment as its `TEMPORAL_RANGE` bucket (new `getSunburstDrillByFilter`). Drill to detail keeps `==` + `grain`, which goes to the backend without `simpleFilterToAdhoc`. The docs now state the Time Range override and the start-instant match for engine-specific grains, as for Timeseries. Regression tests (fail before / pass after): - `superset-frontend/src/components/Chart/DrillBy/DrillByTemporalFilters.test.tsx` › `Sunburst Drill by on a grained segment queries the whole bucket`. It right-clicks a rendered Sunburst, opens `DrillByModal` with those filters and checks `queries[0].filters` in the posted chart-data request, after `simpleFilterToAdhoc` and `convertFilter`. Before: `{col: 'order_date', op: '==', val: 1727740800000}`. - `EchartsSunburst.test.tsx` › `drill by selects a grained bucket with its TEMPORAL_RANGE`, and `preserves the engine grain %s for cross-filter and drill equality` (6 cases). Backend pin (passes on the base too; the backend is unchanged here): `test_hours_offset_bound_truncation.py::test_drill_by_bucket_range_returns_the_segment_rows_with_hours_offset` executes that range shape on SQLite, with and without SQL shift, for offsets 0/1/-1, and gets each segment's row count. `DrillByTemporalFilters.test.tsx` › `Drill by drops the grain of an equality filter` pins the shared behavior you described. Siblings checked: - `superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx:86` — not affected: the cross-filter (`getSunburstFilter`) is already a range, sent through `extraFormData`. - `superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx:142` — not affected: Drill to detail (`getSunburstDetailFilter`) filters reach the backend with `grain`. - `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:685` — fixed in 4e8c60e92e: the shared helper returns master's clause again, with no `grain` key; known grains already used a range. - `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:702` — fixed in 4e8c60e92e: same helper, forced-categorical temporal axis. - `superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:156` — not affected: the `xAxisFilters` merge feeds the same `DrillByModal` conversion, covered by the request-path test. - `superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:264` — not affected: `getFiltersFromConfigsByFieldName` uses the same `simpleFilterToAdhoc` conversion. - `docs/docs/using-superset/exploring-data.mdx` — fixed in 4e8c60e92e. **🟡 Epoch values break Timeseries Drill by on adhoc x-axes** — fixed in 4e8c60e92ebbcec7bcc4e296677b80691361090d. `getTemporalXAxisDrillByFilter` sends master's naive ISO datetime again, and its tests are back to master's expectations. Regression tests (fail before / pass after): - `DrillByTemporalFilters.test.tsx` › `Timeseries Drill by on an adhoc x-axis label sends an ISO datetime, not an epoch` (request path; covers the exact and the engine-grain fallback). - `test/Timeseries/EchartsTimeseries.test.tsx` › `temporal drill by compares a naive ISO datetime for grain %s` (2 cases). - `src/Timeseries/EchartsTimeseries.test.tsx` › `drillBy uses the swapped data index for a horizontal time-based axis` and the three `getTemporalXAxisDrillByFilter` exact-match tests, restored to master. Backend evidence (passes on the base too): `test_adhoc_label_drill_comparator_is_not_coerced` compiles a filter on an adhoc x-axis label: an epoch reaches SQL as `= 1785628800000`, an ISO value as a quoted literal. Siblings checked: - Sunburst adhoc temporal levels — fixed in 4e8c60e92e: cross-filter, Drill to detail and Drill by sent an epoch for an adhoc SQL level too, where `target_native_type` is None and nothing converts it. They send the ISO datetime; physical columns keep the epoch. Tests: `EchartsSunburst.test.tsx` › `adhoc temporal levels compare a naive ISO datetime, not an epoch integer`, and `DrillByTemporalFilters.test.tsx` › `Sunburst Drill by on an adhoc temporal segment writes a quoted ISO datetime` (both fail before). - `superset/models/helpers.py` `handle_temporal_value`, engines whose `convert_dttm` returns None — not changed. `TableColumn.type_generic` returns TEMPORAL for every `is_dttm` column, including BIGINT epoch columns, where the integer must pass through. An ISO fallback there would break those columns, so I took your first option. **🟡 Hours-offset fix only covers `==` without SQL shift** — fixed in db78e36ce9ec440f47da952356e43b6ae63978c4, pushed at 17:13:20Z, 20 s before this review was submitted, so the review text describes cfb9605494. The value adjustment gate covers `==`, `!=`, `IN`, `NOT IN`, `<`, `<=`, `>`, `>=`. Regression tests (fail with cfb9605494's `helpers.py`, pass at db78e36ce9 and at the final head, run in Docker): `test_grained_membership_filter_shifts_each_bucket_value` (4 of 8 cases fail before) and `test_grained_drill_matches_every_bucket_with_hours_offset` (96 cases fail before). Siblings checked: - `TableChart.tsx:601` (`IN` + `grain` on the time column) — fixed in db78e36ce9: the `IN` / `NOT IN` cases above. - `plugin-chart-ag-grid-table/src/utils/getCrossFilterDataMask.ts` — fixed in db78e36ce9: same filter shape and backend path. - physical, verbose, `__timestamp`, adhoc and adhoc-label filter paths — fixed in db78e36ce9: each is a parameter of the matrix test. **🟡 Backend change affects every chart's grained filters** — partly done, at 62cef72cee. I retitled the PR to `fix: filter Sunburst temporal segments by bucket and invert Hours offset in grained filters`. The description lists the affected charts (Drill to detail from Timeseries, Mixed Timeseries, Big Number with trendline, Pivot Table and Sunburst; Table and AG Grid Table grained cross-filters) and names the reversal of https://github.com/apache/superset/pull/43173. I kept one PR rather than splitting. **🟢 Timezone check doesn't match pandas** — fixed in 62cef72cee285fa71d1b5d43fe44a69ea48422ff. The gate checks whether the timezone resolves (`_has_valid_dataset_timezone`). With an unresolvable timezone, the SQL shift is skipped on every engine and pandas falls back to the Hours offset, so the bucket value is inverted on both engine groups. Regression test: `test_grained_drill_with_invalid_dataset_timezone_uses_hours_offset` (12 cases, all fail before, pass after; Docker). Siblings checked: - `superset/connectors/sqla/models.py:1404` and `:2280` (SQL shift gates) — not affected: they skip the shift for any configured timezone, which the new gate accounts for. - `superset/models/helpers.py` `get_time_filter` — not affected: it already falls back to the Hours offset on `ZoneInfoNotFoundError`. - `superset/models/helpers.py` `_collect_dttm_labels` / `normalize_dttm_col` — not affected: unshifted labels get the pandas offset, which the test asserts. **🟢 Legacy label fallback false highlighting** — fixed in 4e8c60e92ebbcec7bcc4e296677b80691361090d. Highlighting compares keys level by level (`isSunburstPathSelected`): a segment stays lit only when its key is the selected key or one of its ancestors. Regression test: `transformProps.test.ts` › `a year label is not highlighted by digits inside a selected epoch key` (fails before, passes after). Siblings checked: - `EchartsSunburst.tsx` `isCurrentValueSelected` — not affected: exact `Array.includes` on whole keys. **🟢 Ungrained temporal equality with an Hours offset** — not fixed. I narrowed the docs at `exploring-data.mdx` to say so. **🟢 `base.py:579`** — you are right: `superset/db_engine_specs/base.py` is not changed in this PR. Line 579 is the `supports_temporal_column_shift` default that `helpers.py` reads. My earlier "fixed" for that line was wrong. **🟢 https://github.com/apache/superset/pull/45571 coordination** — open, see Remaining. Validation at 62cef72cee: `npx tsc --noEmit` in the ECharts plugin exit 0; `npx jest --runInBand plugins/plugin-chart-echarts src/components/Chart` 1615 passed, 1 skipped (123 suites); Docker Python 3.11 `pytest -n 2 --dist loadfile` on `test_hours_offset_bound_truncation.py`, `helpers_test.py` and `connectors/sqla/models_test.py` 881 passed; changed-file pre-commit with ruff 0.9.7 exit 0. Remaining: - Drill by on a grained segment: a chart or dashboard Time Range replaces the bucket range, and an engine-specific grain matches only the bucket start. Both are shared with Timeseries Drill by; carrying `grain` through `SimpleAdhocFilter` → `simpleFilterToAdhoc` → `convertFilter` is left for its own PR. - Ungrained temporal equality with an Hours offset compares the stored timestamp (not new, every engine). - Physical temporal columns on engines whose `convert_dttm` returns None still receive the epoch integer in Drill to detail and cross-filters, as on master for every chart's Drill to detail. - The PR is not split; I have not requested a review from the author of https://github.com/apache/superset/pull/43173. - https://github.com/apache/superset/pull/45571 lands first. This PR then merges master, adopts its `timeGrain.ts` (which grains every temporal level when no time column is set), and the stacking resolves the overlapping files. Its broader `helpers.py` change would shift `TEMPORAL_RANGE` + grain twice and should be dropped in favour of this one; I have not edited that branch. -- 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]
