From 5f2553d7c3bb1a1344a6924ca814bbe7658dfcfe Mon Sep 17 00:00:00 2001 From: Erik Ritter Date: Wed, 12 Jan 2022 11:09:59 -0800 Subject: [PATCH] fix: SQL Lab sorting of non-numbers (#18006) --- .../FilterableTable/FilterableTable.test.tsx | 59 +++++++++++++++++++ .../FilterableTable/FilterableTable.tsx | 21 +++++-- 2 files changed, 74 insertions(+), 6 deletions(-) diff --git a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx index 77f7f712f837b..fad9e8245df22 100644 --- a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx +++ b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx @@ -271,4 +271,63 @@ describe('FilterableTable sorting - RTL', () => { expect(gridCells[9]).toHaveTextContent('3439718.0300000007'); expect(gridCells[10]).toHaveTextContent('4528047.219999993'); }); + + it('sorts YYYY-MM-DD properly', () => { + const dsProps = { + orderedColumnKeys: ['ds'], + data: [ + { ds: '2021-01-01' }, + { ds: '2022-01-01' }, + { ds: '2021-01-02' }, + { ds: '2021-01-03' }, + { ds: '2021-12-01' }, + { ds: '2021-10-01' }, + { ds: '2022-01-02' }, + ], + height: 500, + }; + render(); + + const dsColumn = screen.getByRole('columnheader', { name: 'ds' }); + const gridCells = screen.getAllByRole('gridcell'); + + // Original order + expect(gridCells[0]).toHaveTextContent('2021-01-01'); + expect(gridCells[1]).toHaveTextContent('2022-01-01'); + expect(gridCells[2]).toHaveTextContent('2021-01-02'); + expect(gridCells[3]).toHaveTextContent('2021-01-03'); + expect(gridCells[4]).toHaveTextContent('2021-12-01'); + expect(gridCells[5]).toHaveTextContent('2021-10-01'); + expect(gridCells[6]).toHaveTextContent('2022-01-02'); + + // First click to sort ascending + userEvent.click(dsColumn); + expect(gridCells[0]).toHaveTextContent('2021-01-01'); + expect(gridCells[1]).toHaveTextContent('2021-01-02'); + expect(gridCells[2]).toHaveTextContent('2021-01-03'); + expect(gridCells[3]).toHaveTextContent('2021-10-01'); + expect(gridCells[4]).toHaveTextContent('2021-12-01'); + expect(gridCells[5]).toHaveTextContent('2022-01-01'); + expect(gridCells[6]).toHaveTextContent('2022-01-02'); + + // Second click to sort descending + userEvent.click(dsColumn); + expect(gridCells[0]).toHaveTextContent('2022-01-02'); + expect(gridCells[1]).toHaveTextContent('2022-01-01'); + expect(gridCells[2]).toHaveTextContent('2021-12-01'); + expect(gridCells[3]).toHaveTextContent('2021-10-01'); + expect(gridCells[4]).toHaveTextContent('2021-01-03'); + expect(gridCells[5]).toHaveTextContent('2021-01-02'); + expect(gridCells[6]).toHaveTextContent('2021-01-01'); + + // Third click to sort ascending again + userEvent.click(dsColumn); + expect(gridCells[0]).toHaveTextContent('2021-01-01'); + expect(gridCells[1]).toHaveTextContent('2021-01-02'); + expect(gridCells[2]).toHaveTextContent('2021-01-03'); + expect(gridCells[3]).toHaveTextContent('2021-10-01'); + expect(gridCells[4]).toHaveTextContent('2021-12-01'); + expect(gridCells[5]).toHaveTextContent('2022-01-01'); + expect(gridCells[6]).toHaveTextContent('2022-01-02'); + }); }); diff --git a/superset-frontend/src/components/FilterableTable/FilterableTable.tsx b/superset-frontend/src/components/FilterableTable/FilterableTable.tsx index 96bef2d73e3a4..ece8c918e9e03 100644 --- a/superset-frontend/src/components/FilterableTable/FilterableTable.tsx +++ b/superset-frontend/src/components/FilterableTable/FilterableTable.tsx @@ -80,6 +80,10 @@ const JSON_TREE_THEME = { base0E: '#ae81ff', base0F: '#cc6633', }; +// This regex handles all possible number formats in javascript, including ints, floats, +// exponential notation, NaN, and Infinity. +// See https://stackoverflow.com/a/30987109 for more details +const ONLY_NUMBER_REGEX = /^(NaN|-?((\d*\.\d+|\d+)([Ee][+-]?\d+)?|Infinity))$/; const StyledFilterableTable = styled.div` height: 100%; @@ -321,16 +325,21 @@ export default class FilterableTable extends PureComponent< ); } - // Parse any floating numbers so they'll sort correctly - parseFloatingNums = (value: any) => { - const floatValue = parseFloat(value); - return Number.isNaN(floatValue) ? value : floatValue; + // Parse any numbers from strings so they'll sort correctly + parseNumberFromString = (value: string | number | null) => { + if (typeof value === 'string') { + if (ONLY_NUMBER_REGEX.test(value)) { + return parseFloat(value); + } + } + + return value; }; sortResults(sortBy: string, descending: boolean) { return (a: Datum, b: Datum) => { - const aValue = this.parseFloatingNums(a[sortBy]); - const bValue = this.parseFloatingNums(b[sortBy]); + const aValue = this.parseNumberFromString(a[sortBy]); + const bValue = this.parseNumberFromString(b[sortBy]); // equal items sort equally if (aValue === bValue) {