Copilot commented on code in PR #34547:
URL: https://github.com/apache/superset/pull/34547#discussion_r2254928065
##########
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts:
##########
@@ -143,6 +145,73 @@ export default function transformProps(
DEFAULT_ECHARTS_BOUNDS[1];
}
+ // Extract unique values for each axis
+ 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
+ const sortAxisValues = (
+ values: any[],
+ sortConfig: string | undefined,
+ axisLabel: string,
+ ) => {
+ if (!sortConfig) {
+ return values;
+ }
+
+ const isMetricSort = sortConfig.includes('value');
+ const isAscending = sortConfig.includes('asc');
+
+ 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;
+ }
+ });
+
+ values.sort((a, b) => {
+ const keyA = String(a);
+ const keyB = String(b);
+ const sumA = metricSums[keyA] || 0;
+ const sumB = metricSums[keyB] || 0;
Review Comment:
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.
```suggestion
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;
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts:
##########
@@ -143,6 +145,73 @@ export default function transformProps(
DEFAULT_ECHARTS_BOUNDS[1];
}
+ // Extract unique values for each axis
+ 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
Review Comment:
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.
```suggestion
// 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);
});
}
```
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
