fix(scatter): fix ad-hoc metric for pointsize

Author: alexandrusoare

superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.test.ts

@@ -212,20 +212,27 @@ test('Scatter buildQuery should handle js_columns', () => {
   expect(query.columns).toContain('custom_col2');
 });
 
-test('Scatter buildQuery should convert numeric metric value to string', () => {
+test('Scatter buildQuery should handle adhoc SQL metric for point_radius_fixed', () => {
+  const adhocMetric = {
+    label: 'count(*) * 1.1',
+    expressionType: 'SQL' as const,
+    sqlExpression: 'count(*) * 1.1',
+  };
   const formData: DeckScatterFormData = {
     ...baseFormData,
     point_radius_fixed: {
       type: 'metric',
-      value: 123, // numeric metric (edge case)
+      value: adhocMetric,
     },
   };
 
   const queryContext = buildQuery(formData);
   const [query] = queryContext.queries;
 
-  expect(query.metrics).toContain('123');
-  expect(query.orderby).toEqual([['123', false]]);
+  // Should preserve full adhoc metric object (not just the label string)
+  expect(query.metrics).toContainEqual(adhocMetric);
+  // orderby should use the label string
+  expect(query.orderby).toEqual([['count(*) * 1.1', false]]);
 });
 
 test('Scatter buildQuery should set is_timeseries to false', () => {
@@ -310,3 +317,107 @@ test('Scatter buildQuery should deduplicate metrics when radius metric already e
   expect(query.metrics).toEqual(['COUNT(*)', 'AVG(price)']);
   expect(query.metrics).toHaveLength(2);
 });
+
+// Comprehensive point_radius_fixed tests to prevent regressions
+test('Scatter buildQuery should handle adhoc SIMPLE metric for point_radius_fixed', () => {
+  const adhocMetric = {
+    label: 'AVG(population)',
+    expressionType: 'SIMPLE' as const,
+    column: { column_name: 'population' },
+    aggregate: 'AVG' as const,
+  };
+  const formData: DeckScatterFormData = {
+    ...baseFormData,
+    point_radius_fixed: {
+      type: 'metric',
+      value: adhocMetric,
+    },
+  };
+
+  const queryContext = buildQuery(formData);
+  const [query] = queryContext.queries;
+
+  // Should preserve full adhoc metric object
+  expect(query.metrics).toContainEqual(adhocMetric);
+  expect(query.orderby).toEqual([['AVG(population)', false]]);
+});
+
+test('Scatter buildQuery should deduplicate adhoc metrics with same label', () => {
+  const adhocMetric = {
+    label: 'custom_count',
+    expressionType: 'SQL' as const,
+    sqlExpression: 'count(*) * 2',
+  };
+  const formData: DeckScatterFormData = {
+    ...baseFormData,
+    metrics: [adhocMetric], // Already has this metric
+    point_radius_fixed: {
+      type: 'metric',
+      value: adhocMetric, // Same metric for radius
+    },
+  };
+
+  const queryContext = buildQuery(formData);
+  const [query] = queryContext.queries;
+
+  // Should not duplicate the metric
+  expect(query.metrics).toHaveLength(1);
+  expect(query.metrics).toContainEqual(adhocMetric);
+});
+
+test('Scatter buildQuery should handle fixed type with string value correctly', () => {
+  const formData: DeckScatterFormData = {
+    ...baseFormData,
+    point_radius_fixed: {
+      type: 'fix',
+      value: '2500',
+    },
+  };
+
+  const queryContext = buildQuery(formData);
+  const [query] = queryContext.queries;
+
+  // Fixed values should NOT be added to metrics
+  expect(query.metrics).toEqual([]);
+  expect(query.orderby).toEqual([]);
+});
+
+test('Scatter buildQuery should handle undefined value in metric type gracefully', () => {
+  const formData: DeckScatterFormData = {
+    ...baseFormData,
+    point_radius_fixed: {
+      type: 'metric',
+      value: undefined,
+    },
+  };
+
+  const queryContext = buildQuery(formData);
+  const [query] = queryContext.queries;
+
+  // Should not add anything when value is undefined
+  expect(query.metrics).toEqual([]);
+  expect(query.orderby).toEqual([]);
+});
+
+test('Scatter buildQuery should preserve adhoc metric with custom label', () => {
+  const adhocMetric = {
+    label: 'My Custom Metric',
+    expressionType: 'SQL' as const,
+    sqlExpression: 'SUM(revenue) / COUNT(*)',
+    hasCustomLabel: true,
+  };
+  const formData: DeckScatterFormData = {
+    ...baseFormData,
+    point_radius_fixed: {
+      type: 'metric',
+      value: adhocMetric,
+    },
+  };
+
+  const queryContext = buildQuery(formData);
+  const [query] = queryContext.queries;
+
+  // Should preserve full metric including hasCustomLabel
+  expect(query.metrics).toContainEqual(adhocMetric);
+  expect(query.orderby).toEqual([['My Custom Metric', false]]);
+});

superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts

@@ -19,6 +19,8 @@
 import {
   buildQueryContext,
   ensureIsArray,
+  getMetricLabel,
+  QueryFormMetric,
   QueryFormOrderBy,
   SqlaFormData,
   QueryFormColumn,
@@ -31,16 +33,15 @@ import {
 } from '../spatialUtils';
 import {
   addJsColumnsToColumns,
-  processMetricsArray,
   addTooltipColumnsToQuery,
 } from '../buildQueryUtils';
-import { isMetricValue, extractMetricKey } from '../utils/metricUtils';
+import { isMetricValue } from '../utils/metricUtils';
 
 export interface DeckScatterFormData
   extends Omit<SpatialFormData, 'color_picker'>, SqlaFormData {
   point_radius_fixed?: {
     type?: 'fix' | 'metric';
-    value?: string | number;
+    value?: QueryFormMetric;
   };
   multiplier?: number;
   point_unit?: string;
@@ -82,26 +83,29 @@ export default function buildQuery(formData: DeckScatterFormData) {
 
       // Only add metric if point_radius_fixed is a metric type
       const isMetric = isMetricValue(point_radius_fixed);
-      const metricValue = isMetric
-        ? extractMetricKey(point_radius_fixed?.value)
-        : null;
+      // Preserve full metric value (string for saved metrics, object for adhoc)
+      const metricValue = isMetric ? point_radius_fixed?.value : null;
 
       // Preserve existing metrics and only add radius metric if it's metric-based
       const existingMetrics = baseQueryObject.metrics || [];
-      const radiusMetrics = processMetricsArray(
-        metricValue ? [metricValue] : [],
+      // Deduplicate metrics using getMetricLabel for comparison
+      const existingLabels = new Set(
+        existingMetrics.map(m => getMetricLabel(m)),
       );
-      // Deduplicate metrics to avoid adding the same metric twice
-      const metricsSet = new Set([...existingMetrics, ...radiusMetrics]);
-      const metrics = Array.from(metricsSet);
+      const metrics =
+        metricValue && !existingLabels.has(getMetricLabel(metricValue))
+          ? [...existingMetrics, metricValue]
+          : [...existingMetrics];
+
       const filters = addSpatialNullFilters(
         spatial,
         ensureIsArray(baseQueryObject.filters || []),
       );
 
+      // orderby needs string label, not the full metric object
       const orderby =
         isMetric && metricValue
-          ? ([[metricValue, false]] as QueryFormOrderBy[])
+          ? ([[getMetricLabel(metricValue), false]] as QueryFormOrderBy[])
           : (baseQueryObject.orderby as QueryFormOrderBy[]) || [];
 
       return [

superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/buildQueryUtils.ts

@@ -92,10 +92,6 @@ export function addColumnsIfNotExists(
   return result;
 }
 
-export function processMetricsArray(metrics: (string | undefined)[]): string[] {
-  return metrics.filter((metric): metric is string => Boolean(metric));
-}
-
 export function extractTooltipColumns(tooltipContents?: unknown[]): string[] {
   if (!Array.isArray(tooltipContents) || !tooltipContents.length) {
     return [];