SEPURI-SAI-KRISHNA commented on PR #43222:
URL: https://github.com/apache/superset/pull/43222#issuecomment-5565519832
Rebased on master. Flagging a scope change, because the diff now touches a
file the original PR had nothing to do with.
**The bug gained a second home while this sat.** #43407 migrated Global
Async Queries onto the Global Task Framework and added
`_inject_contribution_totals` in `superset/tasks/async_queries.py`, which
carries its own copy of the predicate:
```python
totals = {col: df[col].sum() for col in df.columns if df[col].dtype.kind in
"biufc"}
```
That function's own docstring says it produces "the same result the
synchronous `ensure_totals_available` produces, but reading the cache the
prerequisite populated instead of re-running the totals query." With two
independent copies of the test, the two paths disagree on identical data: a
`NUMERIC` column that psycopg2 hands back as `decimal.Decimal` has an object
dtype, so the async path drops the metric from the totals dict and
`contribution()` reads back `None` and writes a zero. Same silent 0% as the
sync path, reachable only with `GLOBAL_ASYNC_QUERIES` on.
Fixing one and not the other would have left the async path quietly wrong,
so this now covers both call sites.
**Sharing the predicate rather than copying it a third time.**
`_is_summable` is renamed to `is_summable` and imported by `async_queries.py`.
That follows what `normalize_contribution_totals` already does in the same
module, a shared helper
living in `query_context_processor.py`, imported by `query_context.py`, so
the convention is established rather than invented here. The import is
function-local, next to the existing `QueryCacheManager` one, because
`async_queries.py` deliberately keeps every `superset.common.*` import out of
module scope (they are all under `TYPE_CHECKING`).
**One rebase adaptation worth naming.** `ensure_totals_available` grew a
`(queries_needing_totals, totals_idx)` signature and delegates to
`prepare_contribution_totals()` when called bare, so the existing test's plain
`MagicMock` context no longer unpacked. It uses master's own
`_wire_contribution_totals()` helper now instead of hand-rolled wiring.
Verified locally: 94 tests pass across both files, and pre-commit is clean.
I also mutation-tested both guards, putting `dtype.kind in "biufc"` back at
either call site fails the corresponding test, so neither is passing for an
incidental reason.
**On the one red check.** `test-postgres-hive` is red for an infrastructure
reason, not a finding against this diff. It failed at 43s in step 6, "Start
hadoop and hive", and steps 7-11, Setup Python, Setup Postgres, Start Celery
worker, Python unit tests (PostgreSQL), Upload code coverage, were all skipped,
so no test ran against this branch at all. The rest of the board is green: 62
passing, 12 skipped, that one failure. The same job passes in roughly four
minutes on my other open PRs and on recent master commits. A committer
re-running that single job should clear it; I do not have permission to re-run
it myself.
--
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]
