Copilot commented on code in PR #36214: URL: https://github.com/apache/superset/pull/36214#discussion_r2550624847
########## superset-frontend/plugins/plugin-chart-echarts/spec/Timeseries/Scatter/transformProps.test.ts: ########## @@ -0,0 +1,183 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { ChartProps, SMART_DATE_ID } from '@superset-ui/core'; +import transformProps from '../../../src/Timeseries/transformProps'; +import { DEFAULT_FORM_DATA } from '../../../src/Timeseries/constants'; +import { + EchartsTimeseriesSeriesType, + EchartsTimeseriesFormData, + EchartsTimeseriesChartProps, +} from '../../../src/Timeseries/types'; +import { GenericDataType } from '@apache-superset/core/api/core'; +import { + D3_FORMAT_OPTIONS, + D3_TIME_FORMAT_OPTIONS, +} from '@superset-ui/chart-controls'; +import { supersetTheme } from '@apache-superset/core/ui'; + +describe('Scatter Chart X-axis Time Formatting', () => { + const baseFormData: EchartsTimeseriesFormData = { + ...DEFAULT_FORM_DATA, + colorScheme: 'supersetColors', + datasource: '1__table', + granularity_sqla: '__timestamp', + metric: ['column 1'], + groupby: [], + viz_type: 'echarts_timeseries_scatter', + seriesType: EchartsTimeseriesSeriesType.Scatter, + }; + + const timeseriesData = [ + { + data: [ + { column_1: 0.72099, __timestamp: 1609459200000 }, + { column_1: 0.77954, __timestamp: 1612137600000 }, + { column_1: 2.83434, __timestamp: 1614556800000 }, + ], + colnames: ['column_1', '__timestamp'], + coltypes: [GenericDataType.Numeric, GenericDataType.Temporal], + }, + ]; + + const baseChartPropsConfig = { + width: 800, + height: 600, + queriesData: timeseriesData, + theme: supersetTheme, + }; + + test('xAxisTimeFormat has no default formatter', () => { + const chartProps = new ChartProps({ + ...baseChartPropsConfig, + formData: baseFormData, + }); + + const transformedProps = transformProps( + // @ts-ignore + chartProps as EchartsTimeseriesChartProps, + ); + + expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel'); + const xAxis = transformedProps.echartOptions.xAxis as any; + expect(xAxis.axisLabel).toHaveProperty('formatter'); + expect(xAxis.axisLabel.formatter).toBeUndefined(); + }); + + test.each(D3_TIME_FORMAT_OPTIONS.map(([id]) => id))( + 'should handle %s format', + format => { + const chartProps = new ChartProps({ + ...baseChartPropsConfig, + formData: { + ...baseFormData, + xAxisTimeFormat: format, + }, + }); + + const transformedProps = transformProps( + // @ts-ignore + chartProps as EchartsTimeseriesChartProps, + ); + + const xAxis = transformedProps.echartOptions.xAxis as any; + expect(xAxis.axisLabel).toHaveProperty('formatter'); + if (format === SMART_DATE_ID) { + expect(xAxis.axisLabel.formatter).toBeUndefined(); + } else { + expect(typeof xAxis.axisLabel.formatter).toBe('function'); + expect(xAxis.axisLabel.formatter.id).toBe(format); + } + }, + ); +}); + +describe('Scatter Chart X-axis Number Formatting', () => { + const baseFormData: EchartsTimeseriesFormData = { + ...DEFAULT_FORM_DATA, + colorScheme: 'supersetColors', + datasource: '1__table', + metric: ['column_1'], + x_axis: 'column_2', + groupby: [], + viz_type: 'echarts_timeseries_scatter', + seriesType: EchartsTimeseriesSeriesType.Scatter, + }; + + const timeseriesData = [ + { + data: [ + { column_1: 0.72099, column_2: 3.01699 }, + { column_1: 0.77954, column_2: 3.44802 }, + { column_1: 2.83434, column_2: 3.58095 }, + ], + colnames: ['column_1', 'column_2'], + coltypes: [GenericDataType.Numeric, GenericDataType.Numeric], + }, + ]; + + const baseChartPropsConfig = { + width: 800, + height: 600, + queriesData: timeseriesData, + theme: supersetTheme, + }; + + test('should use SMART_NUMBER as default xAxisNumberFormat', () => { + const chartProps = new ChartProps({ + ...baseChartPropsConfig, + formData: baseFormData, + }); + + const transformedProps = transformProps( + // @ts-ignore + chartProps as EchartsTimeseriesChartProps, + ); + + expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel'); + const xAxis = transformedProps.echartOptions.xAxis as any; + expect(xAxis.axisLabel).toHaveProperty('formatter'); + expect(typeof xAxis.axisLabel.formatter).toBe('function'); + expect(xAxis.axisLabel.formatter.id).toBe('SMART_NUMBER'); + }); + + test.each(D3_FORMAT_OPTIONS.map(([id]) => id))( + 'should handle %s format', + format => { + const chartProps = new ChartProps({ + ...baseChartPropsConfig, + formData: { + ...baseFormData, + xAxisNumberFormat: format, + }, + }); + + const transformedProps = transformProps( + // @ts-ignore + chartProps as EchartsTimeseriesChartProps, + ); + + expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel'); + const xAxis = transformedProps.echartOptions.xAxis as any; + expect(xAxis.axisLabel).toHaveProperty('formatter'); + expect(typeof xAxis.axisLabel.formatter).toBe('function'); + expect(xAxis.axisLabel.formatter.id).toBe(format); + }, + ); +}); Review Comment: This test file appears to be a duplicate of `superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts` which already exists in the codebase. The content is identical. Having duplicate test files in both `spec/` and `test/` directories creates maintenance issues - any updates would need to be synchronized across both files. Please verify which directory should contain the tests for this package and remove the duplicate. Based on the project structure, the `test/` directory already contains similar test files, so this `spec/` version is likely unnecessary. ########## superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts: ########## @@ -483,12 +531,28 @@ export default function transformProps( xAxisDataType === GenericDataType.Temporal ? getTooltipTimeFormatter(tooltipTimeFormat) : String; - const xAxisFormatter = - xAxisDataType === GenericDataType.Temporal - ? getXAxisFormatter(xAxisTimeFormat) - : xAxisDataType === GenericDataType.Numeric - ? getNumberFormatter(xAxisNumberFormat) - : String; + + // For temporal x-axis, keep the existing time formatter behavior. + // For numeric x-axis use a number formatter. Default to SMART_NUMBER if none set. + let xAxisFormatter: + | ((...args: any[]) => string) + | StringConstructor + | undefined; + + if (xAxisDataType === GenericDataType.Temporal) { + xAxisFormatter = getXAxisFormatter(xAxisTimeFormat); + } else if (xAxisDataType === GenericDataType.Numeric) { + // use provided xAxisNumberFormat, fall back to SMART_NUMBER + const numericFormat = xAxisNumberFormat ?? NumberFormats.SMART_NUMBER; + const numericFormatter = getNumberFormatter(numericFormat) as any; + // Ensure formatter.id exists for tests that assert on it + if (!numericFormatter.id) { + numericFormatter.id = numericFormat; + } + xAxisFormatter = numericFormatter; Review Comment: Mutating the formatter object by adding an `id` property after creation is a side effect that could cause issues with shared formatter instances. If `getNumberFormatter` returns a cached or shared formatter instance, adding the `id` property will affect all usages of that formatter. Consider one of these approaches: 1. Create a wrapper function that includes the id: ```typescript const baseFormatter = getNumberFormatter(numericFormat); xAxisFormatter = Object.assign( (...args: any[]) => baseFormatter(...args), { id: numericFormat } ); ``` 2. Or check if the formatter already has an id before adding one: ```typescript if (!numericFormatter.id) { numericFormatter = Object.assign(() => numericFormatter(...arguments), { ...numericFormatter, id: numericFormat }); } ``` ```suggestion xAxisFormatter = Object.assign( (...args: any[]) => numericFormatter(...args), { id: numericFormat } ); } else { xAxisFormatter = numericFormatter; } ``` ########## superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts: ########## @@ -373,6 +390,37 @@ export default function transformProps( } }); + // ----- ensure series data are sorted naturally on the x-value ----- + // Run after all series have been created so each series.data is complete. + series.forEach((s: SeriesOption) => { + const dataArr = (s as any).data; + if (!Array.isArray(dataArr) || dataArr.length <= 1) return; + + (s as any).data = dataArr.sort((row1: any, row2: any) => { + // extract the raw x values (support both [x,y] and { x, y } shapes) + const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x; + const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x; + + // If this chart's x-axis is temporal, coerce to timestamps (numbers) for sorting. + // Fallback to original raw values if parsing fails. + const getComparableX = (raw: any) => { + if (xAxisType === AxisType.Time) { + // If it's already a number, use it. Otherwise try to coerce to Date timestamp. + if (typeof raw === 'number' && isFinite(raw)) return raw; + const parsed = new Date(String(raw)).getTime(); + return isFinite(parsed) ? parsed : String(raw); + } + return raw; + }; + + const x1 = getComparableX(rawX1); + const x2 = getComparableX(rawX2); + + // naturalCompare already prefers numeric comparison when possible + return naturalCompare(x1, x2); + }); Review Comment: The sorting operation is performed by mutating the series data arrays in-place for every series. This could have performance implications for charts with many series and large datasets. Additionally, the sort is performed after annotation series are added (lines 443-495), which means annotation data will also be sorted even though they may not need it. Consider: 1. Checking if the data is already sorted before performing the sort operation 2. Excluding annotation series from sorting (they typically have their own data structure and ordering requirements) 3. If this sorting is only needed for certain series types, add a conditional check For example: ```typescript series.forEach((s: SeriesOption) => { // Skip annotation series - they have their own ordering if ((s as any).markLine || (s as any).markArea) return; const dataArr = (s as any).data; if (!Array.isArray(dataArr) || dataArr.length <= 1) return; // ... rest of sorting logic }); ``` ```suggestion // Helper to check if data is already sorted by x-value function isDataSortedByX( dataArr: any[], getComparableX: (raw: any) => any, ): boolean { for (let i = 1; i < dataArr.length; i++) { const prev = Array.isArray(dataArr[i - 1]) ? dataArr[i - 1][0] : dataArr[i - 1]?.x; const curr = Array.isArray(dataArr[i]) ? dataArr[i][0] : dataArr[i]?.x; if (naturalCompare(getComparableX(prev), getComparableX(curr)) > 0) { return false; } } return true; } series.forEach((s: SeriesOption) => { // Skip annotation series - they have their own ordering if ((s as any).markLine || (s as any).markArea) return; const dataArr = (s as any).data; if (!Array.isArray(dataArr) || dataArr.length <= 1) return; // extract the raw x values (support both [x,y] and { x, y } shapes) const getComparableX = (raw: any) => { if (xAxisType === AxisType.Time) { // If it's already a number, use it. Otherwise try to coerce to Date timestamp. if (typeof raw === 'number' && isFinite(raw)) return raw; const parsed = new Date(String(raw)).getTime(); return isFinite(parsed) ? parsed : String(raw); } return raw; }; if (!isDataSortedByX(dataArr, getComparableX)) { (s as any).data = dataArr.slice().sort((row1: any, row2: any) => { const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x; const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x; const x1 = getComparableX(rawX1); const x2 = getComparableX(rawX2); // naturalCompare already prefers numeric comparison when possible return naturalCompare(x1, x2); }); } ``` ########## superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts: ########## @@ -373,6 +390,37 @@ export default function transformProps( } }); + // ----- ensure series data are sorted naturally on the x-value ----- + // Run after all series have been created so each series.data is complete. + series.forEach((s: SeriesOption) => { + const dataArr = (s as any).data; + if (!Array.isArray(dataArr) || dataArr.length <= 1) return; + + (s as any).data = dataArr.sort((row1: any, row2: any) => { + // extract the raw x values (support both [x,y] and { x, y } shapes) + const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x; + const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x; + + // If this chart's x-axis is temporal, coerce to timestamps (numbers) for sorting. + // Fallback to original raw values if parsing fails. + const getComparableX = (raw: any) => { + if (xAxisType === AxisType.Time) { + // If it's already a number, use it. Otherwise try to coerce to Date timestamp. + if (typeof raw === 'number' && isFinite(raw)) return raw; + const parsed = new Date(String(raw)).getTime(); + return isFinite(parsed) ? parsed : String(raw); Review Comment: The `getComparableX` function can return either a `number` or a `string`, which could lead to inconsistent sorting behavior. When temporal parsing fails for one value but succeeds for another, you'll be comparing numbers with strings using `naturalCompare`. For example: - `raw1 = "invalid-date"` → `getComparableX` returns `"invalid-date"` (string) - `raw2 = "2021-01-01"` → `getComparableX` returns `1609459200000` (number) When these are passed to `naturalCompare`, the number will be converted to a string (`"1609459200000"`), and you'll get lexicographic comparison instead of proper temporal sorting. Consider normalizing the fallback to ensure consistent types: ```typescript const getComparableX = (raw: any) => { if (xAxisType === AxisType.Time) { if (typeof raw === 'number' && isFinite(raw)) return raw; const parsed = new Date(String(raw)).getTime(); // If parsing fails, return a very large number to sort invalid dates last, // or return 0 to sort them first return isFinite(parsed) ? parsed : Number.MAX_SAFE_INTEGER; } return raw; }; ``` ```suggestion // If parsing fails, return a very large number to sort invalid dates last return isFinite(parsed) ? parsed : Number.MAX_SAFE_INTEGER; ``` ########## superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts: ########## @@ -110,6 +110,23 @@ import { getYAxisFormatter, } from '../utils/formatters'; +// ----- natural sort helper ----- +// Try numeric comparison first for numeric-like strings, fallback to localeCompare. +function naturalCompare(a: any, b: any): number { + const sa = a === undefined || a === null ? '' : String(a); + const sb = b === undefined || b === null ? '' : String(b); + const na = Number(sa); + const nb = Number(sb); + + // If both parse as finite numbers, do numeric sort + if (isFinite(na) && isFinite(nb)) { + return na - nb; + } + + // Otherwise fallback to lexicographic + return sa.localeCompare(sb); +} Review Comment: The `naturalCompare` function doesn't handle empty strings correctly when comparing numeric-like values. An empty string converts to `0` when parsed with `Number('')`, which will be treated as a valid finite number and sorted numerically. This could cause unexpected behavior when comparing empty strings with actual zeros or other numeric values. Consider adding an explicit check for empty strings before numeric parsing: ```typescript function naturalCompare(a: any, b: any): number { const sa = a === undefined || a === null ? '' : String(a); const sb = b === undefined || b === null ? '' : String(b); // Handle empty strings explicitly to avoid treating them as 0 if (sa === '' && sb === '') return 0; if (sa === '') return -1; if (sb === '') return 1; const na = Number(sa); const nb = Number(sb); if (isFinite(na) && isFinite(nb)) { return na - nb; } return sa.localeCompare(sb); } ``` -- 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]
