gkneighb commented on PR #43572:
URL: https://github.com/apache/superset/pull/43572#issuecomment-5674127267
@aminghadersohi You're right, and the regression is mine — thanks for
catching it before it merged. Fixed at `a75b6ccd52`.
The defect was in the callers, and your framing ("not a user-requested
override") is exactly it. Four sites passed a hardcoded `order_desc=True` into
the query builder:
- `get_chart_preview.py:272` (ASCII), `:352` (table), `:454` (Vega-Lite)
- `get_chart_data.py:616` (saved-data fallback)
Once the argument began outranking form_data, that literal `True` started
overriding the chart's own flag. All four now forward the chart's direction:
```python
order_desc=form_data.get("order_desc", True),
```
What convinced me this is the right shape rather than special-casing bubble:
the same module already did it this way everywhere else.
`get_chart_data.py:560` (cached form_data) and `:1067` (unsaved form_data) were
already forwarding `form_data.get("order_desc", True)` — which is precisely why
your probe found the compile/cached path ascending and the saved/preview path
descending. The four sites I changed were the outliers, so the split you
measured is now closed in the direction the rest of the module already went.
Regressions through both public paths, as you asked, rather than only the
helper:
- `test_saved_chart_preview_forwards_its_own_sort_direction` — drives
`VegaLitePreviewStrategy.generate()` on a saved bubble chart with `order_desc:
False`, `row_limit: 1`, and asserts the direction reaching
`build_query_context_from_form_data` is `False`.
-
`TestSavedDataFallbackSortDirection::test_fallback_forwards_the_charts_saved_direction`
— drives `get_chart_data` through the tool with a chart that has no
`query_context` (so it takes the fallback) and asserts the same.
- `test_no_explicit_direction_defers_to_the_saved_flag` — the builder
contract the callers now rely on: no explicit argument means the saved flag
stands.
The existing override test stays, since explicit-argument precedence is
still the rule — it just no longer has a caller asserting a direction it never
knew.
One caveat on my own verification, same as last round: this environment
cannot execute the two tool-level test files (app-init conftest fails on an
outdated SQLAlchemy/Flask, and `--noconftest` trips duplicate model
registration), so those two tests are CI-arbitrated. Locally verified: the
builder contract test, the full bubble file (39 passing), and a diff of the
whole `mcp_service` suite against a clean master worktree showing no new
failures. Last round's equivalents did pass in CI, and I'll watch this 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]
