gkneighb commented on PR #42372:
URL: https://github.com/apache/superset/pull/42372#issuecomment-5439549905
@DamianPendrak you're right, and thanks for the concrete repro — that made
it easy to confirm. Fixed in `09dc2cf9af`.
The diagnosis holds up all the way down: `buildQuery` appended the sort key
to `columns`, and `columns` becomes the GROUP BY in `get_sqla_query`
(`models/helpers.py`, `columns = groupby or columns` → `groupby_all_columns`).
So picking a sort column changed the grain from one row per category to one row
per (category, sort column) pair. `transformProps` re-groups and re-sums, which
is exactly why it looks fine — right up until the multiplied row count passes
`row_limit` and the result gets truncated. Then the bars under-report with no
error, which is the worst version of this bug.
**The fix: order by an aggregate instead of widening the GROUP BY.**
`columns` now stays exactly `[x_axis, ...groupby]`, and the sort key
resolves three ways:
| Sort key | ORDER BY |
| --- | --- |
| already a grouping column (X-axis or breakdown) | the column, by label |
| the chart's metric | the metric label |
| any other dataset column | `MIN(col)` as a SIMPLE adhoc metric |
The aggregate goes straight into `orderby` rather than into `metrics` — the
query builder already resolves adhoc metric dicts there via
`adhoc_metric_to_sqla` and dedupes them against existing metric expressions,
and it adds the expression to the SELECT automatically for engines where
`allows_hidden_orderby_agg` is false. So your example becomes:
```sql
SELECT genre, SUM(global_sales)
FROM video_game_sales
GROUP BY genre
ORDER BY MIN(name) ASC, genre ASC
```
One row per genre, so `row_limit` truncates categories rather than silently
eating rows out of them, and Sports stays at 1330.93.
I went this way rather than restricting the options to `{x-axis, metric}`
like the shared `xAxisSortControl` does, because the case this control is for
is a dedicated ordering column — a `step_order` that drives the waterfall's
narrative sequence. That column is functionally dependent on the category, so
`MIN` returns the column's own value and the ordering is exactly what you'd
expect. For a column that *isn't* dependent on the category, "order categories
by their lowest value of X" is at least well-defined and correct, rather than
well-formed and wrong. I've updated the control description to say so.
**Second bug found while fixing this.** The grouped-column check was a raw
`columns.includes(x_axis_sort)` string compare. An adhoc X-axis is an object,
so it never matched, and sorting by the X-axis *itself* duplicated it into
`columns` — changing the grain even in the case that should have been the safe
one. Now compared via `getColumnLabel`, with a test covering it.
**Tests.** The two existing tests that asserted the old contract
(`expect(query.columns).toContain('sort_order')`) are inverted into regression
tests asserting the sort key never joins `columns` and never becomes a chart
metric. Added coverage for the metric key, the X-axis key, the breakdown key,
and the adhoc X-axis. 16/16 Waterfall tests pass.
Verification was at the query level plus the tests above — I haven't re-run
the live `video_game_sales` chart. Happy to grab before/after screenshots if
you'd like them on the PR.
--
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]
