sadpandajoe opened a new pull request, #45178:
URL: https://github.com/apache/superset/pull/45178
### SUMMARY
SQL Lab's row-limit dropdown could silently override a smaller `LIMIT`
already
written in the query, when the query runs asynchronously (Celery).
`ExecuteSqlCommand._run_sql_json_exec_from_scratch`
(`superset/commands/sql_lab/execute.py`)
recomputes `query.limit` as `min(the query's own SQL LIMIT, the dropdown
limit)`
and persists it via `self._query_dao.update(query, {"limit": ...})`, but that
update was never committed before the async executor dispatched a Celery task
via `.delay()`. `@transaction()` on `run()` only commits after the whole
method
returns — by then an idle worker may have already re-fetched the query on its
own connection (`get_query` in `superset/sql_lab.py`). Under READ COMMITTED,
that worker can see the row as it was at creation time: `query.limit` seeded
from the raw dropdown value by `create_query()`
(`superset/sqllab/sqllab_execution_context.py`), not the smaller value just
computed from the query's own `LIMIT`. The dropdown then silently wins
whenever it is the larger of the two — exactly backwards from the documented
behavior ("the smaller limit applies").
This is the same mechanism reported and diagnosed (but never fixed) in
#40070 —
a community member there found that adding a `db.session.commit()` right
after
the same DAO `update()` call fixes it, consistent with a timing-dependent
race
that doesn't reproduce on every setup.
**Fix:** commit immediately after recomputing the limit, before the async
executor can hand off to a worker. This mirrors the early-commit pattern
already used a few lines earlier in the same file's `_save_new_query`, which
has the identical READ-COMMITTED/async justification in its own docstring.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — backend-only timing fix, no UI change.
### TESTING INSTRUCTIONS
```
PYTHONPATH="$PWD:$PWD/superset-core/src" python -m pytest
tests/unit_tests/commands/sql_lab/test_execute.py -v
```
Added
`test_run_sql_json_exec_from_scratch_commits_recomputed_limit_before_dispatch`,
which records the live value of `query.limit` at every `db.session.commit()`
call and asserts the corrected (minimum) value was already committed by the
time the executor dispatches. It fails without the new `db.session.commit()`
call and passes with it.
Also ran the full existing `tests/unit_tests/commands/sql_lab/` and
`tests/unit_tests/sql_lab_test.py` suites (25 tests) — all pass, no
regressions. `ruff check` / `ruff format --diff` clean on both changed files.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
