This is an automated email from the ASF dual-hosted git repository. asoare pushed a commit to branch alexandrusoare/fix/scatterplot-point-size in repository https://gitbox.apache.org/repos/asf/superset.git
commit d0b72584a0de24f0a10626d64abdf7797160e20f Author: alexandrusoare <[email protected]> AuthorDate: Wed Feb 4 15:20:31 2026 +0200 fix(scatter): fix ad-hoc metric for pointsize --- .../src/layers/Scatter/buildQuery.test.ts | 119 ++++++++++++++++++++- .../src/layers/Scatter/buildQuery.ts | 28 ++--- .../src/layers/buildQueryUtils.ts | 4 - 3 files changed, 131 insertions(+), 20 deletions(-) diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.test.ts b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.test.ts index 830d2c26b58..8cf618dcb51 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.test.ts +++ b/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]]); +}); diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts index e1976c6544f..5f94c7e1585 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts +++ b/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 [ diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/buildQueryUtils.ts b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/buildQueryUtils.ts index f61a71ede7c..7a3c20d2422 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/buildQueryUtils.ts +++ b/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 [];
