aminghadersohi commented on PR #45598: URL: https://github.com/apache/superset/pull/45598#issuecomment-6346734980
@rebenitez1802 I found another gap in my previous reply: https://github.com/apache/superset/pull/45598#issuecomment-6346360369 Fixed in `db78e36ce9ec440f47da952356e43b6ae63978c4`: grained `IN` / `NOT IN` filters now undo the display-only Hours offset for **each** bucket value on engines without SQL temporal shifting. Searching the other operators found the same issue in `!=`, `>`, `<`, `>=`, and `<=`; these use the same adjustment. The `==`, SQL-shift-enabled, and dataset-timezone paths retain their behavior. Regression tests in `tests/unit_tests/models/test_hours_offset_bound_truncation.py`: - `test_grained_drill_matches_every_bucket_with_hours_offset`: all eight comparisons, offsets `0`, `+1`, `-1`, `+25`, `-25`, both capability flags, physical/verbose/`__timestamp`/inline-adhoc/adhoc-label paths. Filters consume actual `P1D` grouped bucket values; assertions check counts and row identities, not just generated SQL. - `test_grained_membership_filter_shifts_each_bucket_value`: two nonadjacent buckets plus NULL, `IN` and `NOT IN`, offsets `+1` and `-1`, both flags; checks included/excluded timestamps and NULL handling. - `test_grained_drill_with_dataset_timezone_ignores_hours_offset`: expanded to `IN` / `NOT IN` as timezone-precedence controls. Fail-before/pass-after (Docker, same tests with `helpers.py` from `cfb96054943cbcba14b900bf17c838b4e943a38f`): matrix/timezone selection reported **`96 failed, 316 passed`**, including `IN` and `NOT IN` at both `+1` and `-1` with SQL shifting disabled. The multi-value test reported **`4 failed, 4 passed`**; all four failures were the capability-disabled cases. With the changed helper, the full three-file run reported **`861 passed, 3817 warnings in 28.71s`**, exit 0. Commands/results: ```text # Docker pr45598-python, SUPERSET_TESTENV=true, checkout source/tests copied in 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" # 861 passed; exit 0 # Fail-before selections: -k "matches_every_bucket or dataset_timezone_ignores" # and -k membership_filter_shifts_each_bucket_value # From superset-frontend npx jest --runInBand 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 pre-commit run --files $(git diff --name-only origin/master...HEAD) # exit 0; applicable hooks Passed, including mypy and frontend type-checking ``` Initial test-setup/type-check iterations were corrected before the quoted final runs. I removed the test containers with `docker rm -fv`; `docker ps -a --filter name=pr45598` returned only the header. Siblings checked: - superset/models/helpers.py:5522 — fixed in db78e36ce9ec440f47da952356e43b6ae63978c4 - superset/models/helpers.py:4259 — not affected: the existing list loop already applies the offset inversion per operand; the corrected caller guard enables it for membership filters - superset/models/helpers.py:5366 — not affected: inline adhoc grain resolution reaches the shared guard; the expanded matrix pins all eight comparisons - superset/models/helpers.py:5410 — not affected: adhoc aliases absent from dataset datetime metadata have no pandas offset to invert; label controls pass - superset/models/helpers.py:5444 — not affected: physical, verbose-name, and __timestamp expression resolution reaches the shared guard; each is covered by the matrix - superset/models/helpers.py:4225 — not affected: TEMPORAL_RANGE bypasses operand shifting and retains its separate time-bound adjustment; existing range tests pass - superset/models/helpers.py:5698 — not affected: null checks have no bucket operands; LIKE/ILIKE patterns are not bucket literals; boolean and array operators are not temporal bucket comparisons and are excluded from the adjustment - superset/connectors/sqla/models.py:1398 — not affected: SQL-shift grouping is unchanged; flag-on matrix cases pass - superset/connectors/sqla/models.py:2275 — not affected: adhoc SQL-shift grouping retains its capability/timezone guards; inline/label controls pass - superset/db_engine_specs/base.py:579 — not affected: unchanged declaration of supports_temporal_column_shift = False, not a fix in cfb96054943cbcba14b900bf17c838b4e943a38f; git diff 81df187b05 HEAD -- superset/db_engine_specs/base.py is empty, correcting my previous sibling entry - superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:599 — not affected: its grained IN serialization is valid and reaches the corrected backend; __timestamp matrix cases pin that payload shape - superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCrossFilterDataMask.ts:70 — not affected: both single-cell and grouped-row grained IN serializers reach the corrected backend; single/multi-value regressions cover those shapes - superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx:141 — not affected: both drill actions already emit equality plus grain; equality controls and Sunburst Jest pass - superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:685 — not affected: fallback equality reaches the shared backend; known-grain range behavior is unchanged - superset/common/query_context_factory.py:448 — not affected: only TEMPORAL_RANGE is rewritten by time_range, not the comparisons changed here - superset/mcp_service/dashboard_scope_sql.py:406 — not affected: this other filter_values_handler caller is ungrained and leaves temporal_offset_hours at its zero default Remaining: - Stacking with https://github.com/apache/superset/pull/45571 and adopting getSunburstTimeGrain in its buildQuery are not implemented here; that branch was not changed. - The existing Timeseries known-grain range/time_range overwrite limitation is unchanged. - Live external-engine/browser acceptance was not run; the capability-disabled path was executed using SQLite with supports_temporal_column_shift explicitly set to False. No real-LLM checks were run. -- 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]
