fix: SQL Lab sorting of non-numbers

apache · Jan 12, 2022 · 2159734 · 2159734
1 parent bdc35a2
commit 2159734
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 6 deletions.
diff --git 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(<FilterableTable {...dsProps} />);
+
+    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
@@ -81,6 +81,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%;
@@ -322,16 +326,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) {