[superset] 07/12: fix: SQL Lab sorting of non-numbers (#18006)

[el...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

elizabeth pushed a commit to branch 1.4
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 5f2553d7c3bb1a1344a6924ca814bbe7658dfcfe
Author: Erik Ritter <er...@airbnb.com>
AuthorDate: Wed Jan 12 11:09:59 2022 -0800

    fix: SQL Lab sorting of non-numbers (#18006)
---
 .../FilterableTable/FilterableTable.test.tsx       | 59 ++++++++++++++++++++++
 .../components/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 77f7f71..fad9e82 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(<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
index 96bef2d..ece8c91 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) {