aminghadersohi commented on PR #45598: URL: https://github.com/apache/superset/pull/45598#issuecomment-6346360369
@rebenitez1802 I need to correct two claims in my earlier reply: https://github.com/apache/superset/pull/45598#issuecomment-6345789382 - My TypeScript-pass claim was wrong. At `81df187b05fcfe51bf40ac3bea4a46a6213c8e51`, `npx tsc --noEmit -p plugins/plugin-chart-echarts/tsconfig.json` reproduced `transformProps.ts(318,22): error TS2345: Argument of type 'TimeGranularity' is not assignable to parameter of type '"P1M" | "P3M" | "P1Y"'.` - The Hours-offset change covered only SQL-shift-capable engines. I should have explicitly listed the other engines' grained drill mismatch under Remaining. Fixed in `cfb96054943cbcba14b900bf17c838b4e943a38f`: 1. The Month/Quarter/Year label check uses a type-safe comparison, without an assertion or suppression. 2. Grained equality filters on the non-SQL-shift path subtract the pandas-only Hours offset from the selected bucket before native timestamp coercion. SQL-shift-capable engines retain their existing shifted grouping/comparison expressions. Dataset timezone takes precedence over Hours offset. Adhoc aliases that receive no pandas normalization are left unshifted, rather than subtracting an offset they never received. Regression test: `tests/unit_tests/models/test_hours_offset_bound_truncation.py::test_grained_drill_matches_every_bucket_with_hours_offset` covers both capability flags, offsets `0`, `+1`, `-1`, `+25`, `-25`, physical names, verbose names, `__timestamp`, inline adhoc columns, and adhoc labels. It compares every returned bucket's count and row identities, including midnight-boundary rows. `test_grained_drill_with_dataset_timezone_ignores_hours_offset` pins the timezone guard; `test_display_only_bucket_offset_value_formats` covers value formats and null/invalid inputs. Fail-before/pass-after: with the pre-fix `81df187b05fcfe51bf40ac3bea4a46a6213c8e51` backend source mounted over the final tests, the matrix plus timezone tests reported **`16 failed, 38 passed, 67 deselected`**. All 16 failures were nonzero-offset, SQL-shift-disabled physical/verbose/`__timestamp`/inline-adhoc cases. The same tests pass with this commit; the full Docker run below includes them and the flag-on regressions. Commands/results at this head: ```text # superset-frontend npx tsc --noEmit -p plugins/plugin-chart-echarts/tsconfig.json # no diagnostics; exit 0 npx jest --runInBand --verbose plugins/plugin-chart-echarts/test/Sunburst plugins/plugin-chart-echarts/test/utils/xAxisDrillByFilter.test.ts plugins/plugin-chart-echarts/test/Timeseries/EchartsTimeseries.test.tsx # Test Suites: 5 passed, 5 total; Tests: 84 passed, 84 total; exit 0 # Docker, SUPERSET_TESTENV=true, mounted checkout, container pr45598-ci pytest -n 2 --dist loadfile --cache-clear -q -o addopts="" tests/unit_tests/models/test_hours_offset_bound_truncation.py tests/unit_tests/models/helpers_test.py tests/unit_tests/connectors/sqla/models_test.py -m "not manual and not llm_compliance" # 495 passed in 31.33s; exit 0 pre-commit run --files $(git diff --name-only origin/master...HEAD) # exit 0; Type-Checking (Frontend), mypy (main), and all applicable hooks Passed. # Hooks reporting no matching files were Skipped, not counted as passed. ``` The first pre-commit attempt failed on formatting and a test typing error; the quoted result is the rerun after those corrections. Siblings checked: - superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts:313 — fixed in cfb96054943cbcba14b900bf17c838b4e943a38f - superset/models/helpers.py:4147 — fixed in cfb96054943cbcba14b900bf17c838b4e943a38f - superset/models/helpers.py:5366 — fixed in cfb96054943cbcba14b900bf17c838b4e943a38f - superset/models/helpers.py:5444 — fixed in cfb96054943cbcba14b900bf17c838b4e943a38f - superset/models/helpers.py:5410 — not affected: unknown adhoc aliases have no pandas Hours offset; the matrix pins their unchanged bucket/count behavior with both capability flags - superset/models/helpers.py:4225 — not affected: TEMPORAL_RANGE returns before the equality-value adjustment; existing range-bound tests pass - superset/connectors/sqla/models.py:1398 — not affected: the SQL-shift-enabled grouping path is unchanged and covered by the flag-on matrix and existing offset tests - superset/connectors/sqla/models.py:2275 — not affected: adhoc SQL-shift grouping retains its existing capability/timezone guards - superset/db_engine_specs/base.py:579 — fixed in cfb96054943cbcba14b900bf17c838b4e943a38f - superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx:141 — not affected: Drill to detail already emits epoch equality plus grain and reaches the corrected backend - superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx:151 — not affected: Drill by uses the same equality serializer and backend path - superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:685 — not affected: fallback equality reaches the shared backend; known-grain ranges remain unchanged - superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:702 — not affected: the second shared-helper caller retains the same serialization; Timeseries Jest tests pass - superset/common/query_context_factory.py:448 — not affected: only TEMPORAL_RANGE is rewritten by time_range; equality is preserved - superset/common/query_context_processor.py:549 — not affected: sync/async query consumers delegate to the same datasource result builder - superset/charts/data/api.py:228 — not affected: authorized chart requests use QueryContext and the shared datasource filter builder - superset/mcp_service/chart/chart_helpers.py:1312 — not affected: builds QueryContext rather than a separate filter implementation - superset/mcp_service/dashboard_scope_sql.py:406 — not affected: its ungrained direct coercion call retains the default zero adjustment - superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/buildQuery.ts — not affected: no custom grain-selection branch exists at this head; sibling integration remains below Remaining: stacking with https://github.com/apache/superset/pull/45571 and adopting `getSunburstTimeGrain` in its `buildQuery` are not complete. I did not change that branch. Remaining: the pre-existing Timeseries known-grain Drill by range/time_range behavior is unchanged. Browser acceptance and live Trino/BigQuery/Snowflake/MySQL execution were not run; the non-SQL-shift regression executes SQLite with the capability explicitly disabled. -- 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]
