Re: [PR] fix(heatmap): preserve axis order when data combinations are missing [superset]

[via GitHub]

korbit-ai[bot] commented on code in PR #34547:
URL: https://github.com/apache/superset/pull/34547#discussion_r2253269216


##########
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;
+        }
+      });

Review Comment:
   ### Incorrect Metric Aggregation for Sorting <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The metric-based sorting implementation assumes summing is the appropriate 
aggregation method for all metrics, which might not be correct for metrics like 
averages, minimums, or maximums.
   
   
   ###### Why this matters
   Using sum as the only aggregation method could lead to incorrect axis 
ordering when the metric represents an average, minimum, maximum, or other 
non-summable values.
   
   ###### Suggested change ∙ *Feature Preview*
   Modify the code to respect the metric's aggregation type by adding an 
aggregation parameter and implementing appropriate aggregation methods:
   ```typescript
   const aggregateMetricValues = (values: number[], aggregationType: string) => 
{
     switch (aggregationType) {
       case 'avg':
         return values.reduce((sum, val) => sum + val, 0) / values.length;
       case 'min':
         return Math.min(...values);
       case 'max':
         return Math.max(...values);
       case 'sum':
       default:
         return values.reduce((sum, val) => sum + val, 0);
     }
   };
   
   if (isMetricSort) {
     const metricValues: Record<string, number[]> = {};
     data.forEach(row => {
       const axisValue = row[axisLabel];
       const metricValue = row[metricLabel];
       if (typeof metricValue === 'number' && axisValue != null) {
         const key = String(axisValue);
         metricValues[key] = metricValues[key] || [];
         metricValues[key].push(metricValue);
       }
     });
   
     const metricSums: Record<string, number> = {};
     Object.entries(metricValues).forEach(([key, values]) => {
       metricSums[key] = aggregateMetricValues(values, metric.aggregationType);
     });
   }
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c95cc65f-d615-487f-b49b-bc25520bd724/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c95cc65f-d615-487f-b49b-bc25520bd724?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c95cc65f-d615-487f-b49b-bc25520bd724?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c95cc65f-d615-487f-b49b-bc25520bd724?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c95cc65f-d615-487f-b49b-bc25520bd724)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:879daad4-de0c-473c-8702-a3ab849d3b86 -->
   
   
   [](879daad4-de0c-473c-8702-a3ab849d3b86)



-- 
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