Skip to content

fix(heatmap): preserve axis order when data combinations are missing - #34547

Closed
rusackas wants to merge 2 commits into
masterfrom
fix-33245-heatmap-axis-sorting
Closed

fix(heatmap): preserve axis order when data combinations are missing#34547
rusackas wants to merge 2 commits into
masterfrom
fix-33245-heatmap-axis-sorting

Conversation

@rusackas

@rusackas rusackas commented Aug 5, 2025

Copy link
Copy Markdown
Member

Summary

This PR fixes issue #33245 where the heatmap chart's y-axis would display in random order when some data combinations were missing. Starting in version 4.1, the chart lost the ability to maintain proper axis ordering in these cases.

Root Cause

The new heatmap chart uses server-side ordering via SQL ORDER BY clauses. However, ECharts automatically creates axis categories from the data it receives, which doesn't preserve the intended order when data is sparse.

Solution

The fix explicitly sets axis categories in the correct order by:

  1. Extracting unique values for each axis from the data
  2. Sorting them according to the user's sorting configuration (alphabetical or by metric value)
  3. Explicitly setting the axis data in the ECharts configuration

This ensures both axes maintain their proper order regardless of missing data combinations.

Test plan

Added comprehensive unit tests that verify:

  • Axes maintain alphabetical order when data is missing
  • Descending order works correctly
  • Sorting by metric value works as expected
  • Numeric values are sorted numerically (not as strings)
  • Original order is preserved when no sorting is specified
  • Null/undefined values are handled correctly

Manual Testing

  1. Create a heatmap chart with sparse data (e.g., day of week vs hour with some combinations missing)
  2. Set both X and Y axis sorting to "Axis ascending"
  3. Verify both axes display in the correct order
  4. Test with different sorting options (descending, by metric value)

Related Issues

🤖 Generated with Claude Code

Fixes #33245

The heatmap chart was not preserving the proper axis order when some data
combinations were missing. This occurred because ECharts automatically
creates axis categories from the data it receives, ignoring the SQL ORDER BY
applied on the backend.

Solution: Explicitly set the axis categories in the correct order by:
1. Extracting unique values for each axis from the data
2. Sorting them according to the user's sorting configuration
3. Explicitly setting the axis data in the ECharts configuration

This ensures both axes maintain their proper order regardless of missing data.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@dosubot dosubot Bot added the viz:charts:heatmap Related to the Heatmap chart label Aug 5, 2025

@korbit-ai korbit-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Status
Functionality Incorrect Metric Aggregation for Sorting ▹ view 🧠 Not in standard
Files scanned
File Path Reviewed
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

Comment on lines +165 to +175
if (isMetricSort) {
// Create a map of axis value to metric sum for sorting by metric
const metricSums: Record<string, number> = {};
data.forEach(row => {
const axisValue = row[axisLabel];
const metricValue = row[metricLabel];
if (typeof metricValue === 'number' && axisValue != null) {
const key = String(axisValue);
metricSums[key] = (metricSums[key] || 0) + metricValue;
}
});

This comment was marked as resolved.

@rusackas rusackas added the AI 🤖 generated by AI label Aug 5, 2025
@rusackas
rusackas requested a review from Copilot August 5, 2025 17:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a heatmap chart axis ordering issue where y-axis values would display in random order when data combinations were missing. The fix ensures proper axis ordering by explicitly setting axis categories in the correct order based on user sorting configuration.

  • Adds logic to extract unique axis values and sort them according to user preferences (alphabetical, by metric value, etc.)
  • Explicitly sets axis data in ECharts configuration to preserve intended order regardless of sparse data
  • Includes comprehensive test coverage for various sorting scenarios and edge cases

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts Implements axis sorting logic and explicitly sets axis data in ECharts configuration
superset-frontend/plugins/plugin-chart-echarts/test/Heatmap/transformProps.test.ts Adds comprehensive unit tests for axis ordering scenarios
Comments suppressed due to low confidence (1)

superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts:154

  • The parameter type 'any[]' is too generic. Consider using a more specific type like 'unknown[]' or define a union type for the expected value types (string | number | null | undefined)[].
    values: any[],

const xValues = Array.from(new Set(data.map(row => row[xAxisLabel])));
const yValues = Array.from(new Set(data.map(row => row[yAxisLabel])));

// Sort axis values based on configuration

Copilot AI Aug 5, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sortAxisValues function is quite large (58 lines) and handles multiple responsibilities. Consider extracting the sorting logic into separate helper functions for metric-based sorting and alphabetical/numeric sorting to improve readability and maintainability.

Suggested change
// Sort axis values based on configuration
// Sort axis values based on configuration
// Helper function for metric-based sorting
function sortByMetric(
values: any[],
axisLabel: string,
metricLabel: string,
isAscending: boolean,
data: Record<string, any>[],
) {
// Create a map of axis value to metric sum for sorting by metric
const metricSums: Record<string, number> = {};
data.forEach(row => {
const axisValue = row[axisLabel];
const metricValue = row[metricLabel];
if (typeof metricValue === 'number' && axisValue != null) {
const key = String(axisValue);
metricSums[key] = (metricSums[key] || 0) + metricValue;
}
});
return values.slice().sort((a, b) => {
const keyA = String(a);
const keyB = String(b);
const sumA = metricSums[keyA] || 0;
const sumB = metricSums[keyB] || 0;
return isAscending ? sumA - sumB : sumB - sumA;
});
}
// Helper function for alphabetical/numeric sorting
function sortAlphabeticallyOrNumerically(
values: any[],
isAscending: boolean,
) {
return values.slice().sort((a, b) => {
// Handle null/undefined values
if (a === null || a === undefined) return isAscending ? -1 : 1;
if (b === null || b === undefined) return isAscending ? 1 : -1;
// Convert to strings for comparison
const strA = String(a);
const strB = String(b);
// Try numeric comparison first
const numA = Number(strA);
const numB = Number(strB);
if (!Number.isNaN(numA) && !Number.isNaN(numB)) {
return isAscending ? numA - numB : numB - numA;
}
// Fall back to string comparison
return isAscending
? strA.localeCompare(strB)
: strB.localeCompare(strA);
});
}

Copilot uses AI. Check for mistakes.
Comment on lines +172 to +181
const key = String(axisValue);
metricSums[key] = (metricSums[key] || 0) + metricValue;
}
});

values.sort((a, b) => {
const keyA = String(a);
const keyB = String(b);
const sumA = metricSums[keyA] || 0;
const sumB = metricSums[keyB] || 0;

Copilot AI Aug 5, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String conversion is performed inside the forEach loop for every data row. Consider using a Map with proper key handling or pre-process the conversion to avoid repeated String() calls on the same values.

Suggested change
const key = String(axisValue);
metricSums[key] = (metricSums[key] || 0) + metricValue;
}
});
values.sort((a, b) => {
const keyA = String(a);
const keyB = String(b);
const sumA = metricSums[keyA] || 0;
const sumB = metricSums[keyB] || 0;
const key = valueToStringMap.get(axisValue);
if (key !== undefined) {
metricSums[key] = (metricSums[key] || 0) + metricValue;
}
}
});
values.sort((a, b) => {
const keyA = valueToStringMap.get(a);
const keyB = valueToStringMap.get(b);
const sumA = keyA ? metricSums[keyA] || 0 : 0;
const sumB = keyB ? metricSums[keyB] || 0 : 0;

Copilot uses AI. Check for mistakes.
- Fix undefined borderColor error by providing fallback to 'transparent'
- Fix axis sorting by using correct camelCase field names (sortXAxis/sortYAxis)
- Fix sorting direction logic by using endsWith('asc') instead of includes('asc')
- Fix TypeScript errors by removing unused import and handling undefined groupby
- All heatmap tests now passing

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 3, 2025

Copy link
Copy Markdown
Contributor

⚠️ DEPRECATED WORKFLOW ⚠️

@sfirke This workflow is deprecated! Please use the new Superset Showtime system instead:

Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

@sfirke sfirke added the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Oct 3, 2025
@github-actions

github-actions Bot commented Oct 3, 2025

Copy link
Copy Markdown
Contributor

@sfirke Ephemeral environment spinning up at http://44.242.192.175:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

@sfirke

sfirke commented Oct 7, 2025

Copy link
Copy Markdown
Member

Thanks for trying to work on this! I deployed this branch into my test environment. The problematic heatmaps now don't render any data at all, everything on the x-axis is NULL (though you can see in the Results below the chart that underlying data is present):

image

I then went back to 6.0.0rc2 and confirmed (a) that the heatmap renders there, so this is a failure on this fix branch (b) that it renders with the y-axis categories in the wrong order = the original sorting bug persists.

@sfirke sfirke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it's not rendering the heatmap in question - makes me wonder if the tests are invalid or just missing something about my case.

@rusackas

Copy link
Copy Markdown
Member Author

Superseded by #36302

@rusackas rusackas closed this Dec 16, 2025
@rusackas
rusackas deleted the fix-33245-heatmap-axis-sorting branch March 6, 2026 17:01
@mistercrunch mistercrunch removed the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI 🤖 generated by AI plugins size/L viz:charts:heatmap Related to the Heatmap chart

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Order of secondary heatmap axis varies based on data (started in 4.1)

5 participants