Copilot commented on code in PR #36302:
URL: https://github.com/apache/superset/pull/36302#discussion_r2577288714
##########
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts:
##########
@@ -38,13 +39,104 @@ import { HeatmapChartProps, HeatmapTransformedProps } from
'./types';
import { getDefaultTooltip } from '../utils/tooltip';
import { Refs } from '../types';
import { parseAxisBound } from '../utils/controls';
-import { NULL_STRING } from '../constants';
import { getPercentFormatter } from '../utils/formatters';
type EChartsOption = ComposeOption<HeatmapSeriesOption>;
const DEFAULT_ECHARTS_BOUNDS = [0, 200];
+/**
+ * Extract unique values for an axis from the data.
+ * Filters out null and undefined values.
+ *
+ * @param data - The dataset to extract values from
+ * @param columnName - The column to extract unique values from
+ * @returns Array of unique values from the specified column
+ */
+function extractUniqueValues(
+ data: Record<string, DataRecordValue>[],
+ columnName: string,
+): DataRecordValue[] {
+ const uniqueSet = new Set<DataRecordValue>();
+ data.forEach(row => {
+ const value = row[columnName];
+ if (value !== null && value !== undefined) {
+ uniqueSet.add(value);
+ }
+ });
+ return Array.from(uniqueSet);
+}
+
+/**
+ * Sort axis values based on the sort configuration.
+ * Supports alphabetical (with numeric awareness) and metric value-based
sorting.
+ *
+ * @param values - The unique values to sort
+ * @param data - The full dataset
+ * @param sortOption - Sort option string (e.g., 'alpha_asc', 'value_desc')
+ * @param metricLabel - Label of the metric for value-based sorting
+ * @param axisColumn - Column name for the axis being sorted
+ * @returns Sorted array of values
+ */
+function sortAxisValues(
+ values: DataRecordValue[],
+ data: Record<string, DataRecordValue>[],
+ sortOption: string | undefined,
+ metricLabel: string,
+ axisColumn: string,
+): DataRecordValue[] {
+ if (!sortOption) {
+ // No sorting specified, return values as they appear in the data
+ return values;
+ }
+
+ const isAscending = sortOption.includes('asc');
+ const isValueSort = sortOption.includes('value');
+
+ if (isValueSort) {
+ // Sort by metric value - aggregate metric values for each axis category
+ const valueMap = new Map<DataRecordValue, number>();
+ data.forEach(row => {
+ const axisValue = row[axisColumn];
+ const metricValue = row[metricLabel];
+ if (
+ axisValue !== null &&
+ axisValue !== undefined &&
+ typeof metricValue === 'number'
+ ) {
+ const current = valueMap.get(axisValue) || 0;
+ valueMap.set(axisValue, current + metricValue);
+ }
+ });
+
+ return values.sort((a, b) => {
+ const aValue = valueMap.get(a) || 0;
+ const bValue = valueMap.get(b) || 0;
+ return isAscending ? aValue - bValue : bValue - aValue;
+ });
Review Comment:
[nitpick] The `sort()` method mutates the input array. While this might work
in the current implementation, it's better practice to avoid mutating input
parameters. Consider creating a copy before sorting:
```typescript
return [...values].sort((a, b) => {
```
This prevents potential side effects if the `values` array is used elsewhere.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts:
##########
@@ -144,22 +238,52 @@ export default function transformProps(
DEFAULT_ECHARTS_BOUNDS[1];
}
+ // Extract and sort unique axis values
+ // Use colnames to get the actual column names in the data
+ const xAxisColumnName = colnames[0];
+ const yAxisColumnName = colnames[1];
+
+ const xAxisValues = extractUniqueValues(data, xAxisColumnName);
+ const yAxisValues = extractUniqueValues(data, yAxisColumnName);
+
+ const sortedXAxisValues = sortAxisValues(
+ xAxisValues,
+ data,
+ sortXAxis,
+ metricLabel,
+ xAxisColumnName,
+ );
+ const sortedYAxisValues = sortAxisValues(
+ yAxisValues,
+ data,
+ sortYAxis,
+ metricLabel,
+ yAxisColumnName,
+ );
+
+ // Create lookup maps for axis indices
+ const xAxisIndexMap = new Map<DataRecordValue, number>(
+ sortedXAxisValues.map((value, index) => [value, index]),
+ );
+ const yAxisIndexMap = new Map<DataRecordValue, number>(
+ sortedYAxisValues.map((value, index) => [value, index]),
+ );
+
const series: HeatmapSeriesOption[] = [
{
name: metricLabel,
type: 'heatmap',
- data: data.map(row =>
- colnames.map(col => {
- const value = row[col];
- if (value === null || value === undefined) {
- return NULL_STRING;
- }
- if (typeof value === 'boolean' || typeof value === 'bigint') {
- return String(value);
- }
- return value;
- }),
- ),
+ data: data.map(row => {
+ const xValue = row[xAxisColumnName];
+ const yValue = row[yAxisColumnName];
+ const metricValue = row[metricLabel];
+
+ // Convert to axis indices for ECharts when explicit axis data is
provided
+ const xIndex = xAxisIndexMap.get(xValue) ?? 0;
+ const yIndex = yAxisIndexMap.get(yValue) ?? 0;
+
+ return [xIndex, yIndex, metricValue] as [number, number, any];
Review Comment:
[nitpick] Defaulting to index 0 when a value is not found in the index map
could mask data issues. If `xValue` or `yValue` is null/undefined (which are
filtered out by `extractUniqueValues`), or if there's a data inconsistency, the
point will be silently placed at index 0 instead of being skipped or logged.
Consider either:
1. Filtering out rows with null/undefined axis values before mapping
2. Logging a warning when values are not found in the index map
3. Skipping data points that can't be properly positioned
This will make debugging easier and prevent incorrect data visualization.
```suggestion
data: data.flatMap(row => {
const xValue = row[xAxisColumnName];
const yValue = row[yAxisColumnName];
const metricValue = row[metricLabel];
// Convert to axis indices for ECharts when explicit axis data is
provided
const xIndex = xAxisIndexMap.get(xValue);
const yIndex = yAxisIndexMap.get(yValue);
if (xIndex === undefined || yIndex === undefined) {
// Log a warning for debugging
// eslint-disable-next-line no-console
console.warn(
`Heatmap: Skipping row due to missing axis value. xValue:
${xValue}, yValue: ${yValue}, metricValue: ${metricValue}`,
row,
);
return [];
}
return [[xIndex, yIndex, metricValue] as [number, number, any]];
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts:
##########
@@ -38,13 +39,104 @@ import { HeatmapChartProps, HeatmapTransformedProps } from
'./types';
import { getDefaultTooltip } from '../utils/tooltip';
import { Refs } from '../types';
import { parseAxisBound } from '../utils/controls';
-import { NULL_STRING } from '../constants';
import { getPercentFormatter } from '../utils/formatters';
type EChartsOption = ComposeOption<HeatmapSeriesOption>;
const DEFAULT_ECHARTS_BOUNDS = [0, 200];
+/**
+ * Extract unique values for an axis from the data.
+ * Filters out null and undefined values.
+ *
+ * @param data - The dataset to extract values from
+ * @param columnName - The column to extract unique values from
+ * @returns Array of unique values from the specified column
+ */
+function extractUniqueValues(
+ data: Record<string, DataRecordValue>[],
+ columnName: string,
+): DataRecordValue[] {
+ const uniqueSet = new Set<DataRecordValue>();
+ data.forEach(row => {
+ const value = row[columnName];
+ if (value !== null && value !== undefined) {
+ uniqueSet.add(value);
+ }
+ });
+ return Array.from(uniqueSet);
+}
+
+/**
+ * Sort axis values based on the sort configuration.
+ * Supports alphabetical (with numeric awareness) and metric value-based
sorting.
+ *
+ * @param values - The unique values to sort
+ * @param data - The full dataset
+ * @param sortOption - Sort option string (e.g., 'alpha_asc', 'value_desc')
+ * @param metricLabel - Label of the metric for value-based sorting
+ * @param axisColumn - Column name for the axis being sorted
+ * @returns Sorted array of values
+ */
+function sortAxisValues(
+ values: DataRecordValue[],
+ data: Record<string, DataRecordValue>[],
+ sortOption: string | undefined,
+ metricLabel: string,
+ axisColumn: string,
+): DataRecordValue[] {
+ if (!sortOption) {
+ // No sorting specified, return values as they appear in the data
+ return values;
+ }
+
+ const isAscending = sortOption.includes('asc');
+ const isValueSort = sortOption.includes('value');
+
+ if (isValueSort) {
+ // Sort by metric value - aggregate metric values for each axis category
+ const valueMap = new Map<DataRecordValue, number>();
+ data.forEach(row => {
+ const axisValue = row[axisColumn];
+ const metricValue = row[metricLabel];
+ if (
+ axisValue !== null &&
+ axisValue !== undefined &&
+ typeof metricValue === 'number'
+ ) {
+ const current = valueMap.get(axisValue) || 0;
+ valueMap.set(axisValue, current + metricValue);
+ }
+ });
+
+ return values.sort((a, b) => {
+ const aValue = valueMap.get(a) || 0;
+ const bValue = valueMap.get(b) || 0;
+ return isAscending ? aValue - bValue : bValue - aValue;
+ });
+ }
+
+ // Alphabetical/lexicographic sort
+ return values.sort((a, b) => {
+ // Check if both values are numeric for proper numeric sorting
+ const aNum = typeof a === 'number' ? a : Number(a);
+ const bNum = typeof b === 'number' ? b : Number(b);
+ const aIsNumeric = Number.isFinite(aNum);
+ const bIsNumeric = Number.isFinite(bNum);
+
+ if (aIsNumeric && bIsNumeric) {
+ // Both are numeric, sort numerically
+ return isAscending ? aNum - bNum : bNum - aNum;
+ }
+
+ // At least one is non-numeric, use locale-aware string comparison
+ const aStr = String(a);
+ const bStr = String(b);
+ const comparison = aStr.localeCompare(bStr, undefined, { numeric: true });
+ return isAscending ? comparison : -comparison;
+ });
Review Comment:
[nitpick] The `sort()` method mutates the input array. While this might work
in the current implementation, it's better practice to avoid mutating input
parameters. Consider creating a copy before sorting:
```typescript
return [...values].sort((a, b) => {
```
This prevents potential side effects if the `values` array is used elsewhere.
--
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]
