sadpandajoe commented on code in PR #44588:
URL: https://github.com/apache/superset/pull/44588#discussion_r4110073499
##########
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts:
##########
@@ -266,10 +266,14 @@ export default function transformProps(
metricLabel,
xAxisColumnName,
);
+ // Unlike the X-axis (often a temporal column that already arrives from
+ // the backend in a natural, e.g. chronological, order), the Y-axis
+ // groupby dimension has no such natural ordering to fall back on, so
+ // default it to ascending sort when the user hasn't chosen one.
const sortedYAxisValues = sortAxisValues(
yAxisValues,
data,
- sortYAxis,
+ sortYAxis ?? 'alpha_asc',
Review Comment:
Following up on the ordinal-values point (`Low/Medium/High`, weekdays, etc.)
— dug into this further and it's worth sharing what I found, even though the
final call on next steps is still pending on my end.
**The regression is real but narrower than it might look:**
- Heatmap runs `SELECT x, y, agg FROM (<virtual sql>) GROUP BY x, y` with no
`ORDER BY` when the sort is unset — and SQL doesn't guarantee a subquery's
`ORDER BY` survives an outer `GROUP BY`. Tested this directly with
`Low/Medium/High` data behind a virtual dataset ordered by a sort key: SQLite
already came back alphabetical, DuckDB came back in a different (non-ordinal)
order on every run. Postgres's hash aggregation and MySQL 8 (whose docs say a
derived table's `ORDER BY` is ignored under a grouped outer query) shouldn't
preserve it either.
- More importantly: legacy Heatmap and Heatmap v2 *before* #31752 (Jan 2025)
had `sort_y_axis` as a non-clearable `default: 'alpha_asc'`. Since Explore
persists control defaults into saved `form_data`, any chart created before then
— or migrated via `MigrateHeatmapChart` — should already have `sort_y_axis:
'alpha_asc'` stored explicitly, not unset. Those charts are unaffected by this
fix either way. The exposed set is specifically 5.0+ charts with the Y sort
left unset (or cleared), which narrows things meaningfully from "any existing
heatmap."
So the "previously meaningful order becomes wrong" scenario is real for some
charts, but likely much rarer than it first appears, and on several engines the
*previous* rendering was already effectively arbitrary for this exact case.
Still deciding on the best next step for the "no way to opt back out" part
specifically — there may be a small, low-risk way to give a persistent opt-out
(distinct from the ambiguous unset/cleared state) without touching the
row_limit tradeoff we already discussed. Wanted to get the investigative
findings to you now rather than sit on them while that's still being decided.
--
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]
