fix(sqllab): Floating numbers not sorting correctly in result column (#…

…17573) * Floating nums now sorting correctly with parseFloatingNums function * Floating numbers AND strings now sorting correctly, +locale comparison * Added NULL handling back to sort function * Moved parseFloatingNums outside of sortResults * Removed localeCompare and added testing * Add equality check back to sort function * Added floatValue nit
apache · Dec 4, 2021 · 05752e3 · 05752e3
1 parent 1377465
commit 05752e3
Show file tree

Hide file tree

Showing 2 changed files with 201 additions and 4 deletions.
diff --git a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx
@@ -22,6 +22,8 @@ import { styledMount as mount } from 'spec/helpers/theming';
 import FilterableTable, {
   MAX_COLUMNS_FOR_TABLE,
 } from 'src/components/FilterableTable/FilterableTable';
+import { render, screen } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
 
 describe('FilterableTable', () => {
   const mockedProps = {
@@ -84,3 +86,189 @@ describe('FilterableTable', () => {
     expect(wrapper.find('.ReactVirtualized__Table__row')).toExist();
   });
 });
+
+describe('FilterableTable sorting - RTL', () => {
+  it('sorts strings correctly', () => {
+    const stringProps = {
+      orderedColumnKeys: ['a'],
+      data: [{ a: 'Bravo' }, { a: 'Alpha' }, { a: 'Charlie' }],
+      height: 500,
+    };
+    render(<FilterableTable {...stringProps} />);
+
+    const stringColumn = screen.getByRole('columnheader', { name: 'a' });
+    const gridCells = screen.getAllByRole('gridcell');
+
+    // Original order
+    expect(gridCells[0]).toHaveTextContent('Bravo');
+    expect(gridCells[1]).toHaveTextContent('Alpha');
+    expect(gridCells[2]).toHaveTextContent('Charlie');
+
+    // First click to sort ascending
+    userEvent.click(stringColumn);
+    expect(gridCells[0]).toHaveTextContent('Alpha');
+    expect(gridCells[1]).toHaveTextContent('Bravo');
+    expect(gridCells[2]).toHaveTextContent('Charlie');
+
+    // Second click to sort descending
+    userEvent.click(stringColumn);
+    expect(gridCells[0]).toHaveTextContent('Charlie');
+    expect(gridCells[1]).toHaveTextContent('Bravo');
+    expect(gridCells[2]).toHaveTextContent('Alpha');
+
+    // Third click to sort ascending again
+    userEvent.click(stringColumn);
+    expect(gridCells[0]).toHaveTextContent('Alpha');
+    expect(gridCells[1]).toHaveTextContent('Bravo');
+    expect(gridCells[2]).toHaveTextContent('Charlie');
+  });
+
+  it('sorts integers correctly', () => {
+    const integerProps = {
+      orderedColumnKeys: ['b'],
+      data: [{ b: 10 }, { b: 0 }, { b: 100 }],
+      height: 500,
+    };
+    render(<FilterableTable {...integerProps} />);
+
+    const integerColumn = screen.getByRole('columnheader', { name: 'b' });
+    const gridCells = screen.getAllByRole('gridcell');
+
+    // Original order
+    expect(gridCells[0]).toHaveTextContent('10');
+    expect(gridCells[1]).toHaveTextContent('0');
+    expect(gridCells[2]).toHaveTextContent('100');
+
+    // First click to sort ascending
+    userEvent.click(integerColumn);
+    expect(gridCells[0]).toHaveTextContent('0');
+    expect(gridCells[1]).toHaveTextContent('10');
+    expect(gridCells[2]).toHaveTextContent('100');
+
+    // Second click to sort descending
+    userEvent.click(integerColumn);
+    expect(gridCells[0]).toHaveTextContent('100');
+    expect(gridCells[1]).toHaveTextContent('10');
+    expect(gridCells[2]).toHaveTextContent('0');
+
+    // Third click to sort ascending again
+    userEvent.click(integerColumn);
+    expect(gridCells[0]).toHaveTextContent('0');
+    expect(gridCells[1]).toHaveTextContent('10');
+    expect(gridCells[2]).toHaveTextContent('100');
+  });
+
+  it('sorts floating numbers correctly', () => {
+    const floatProps = {
+      orderedColumnKeys: ['c'],
+      data: [{ c: 45.67 }, { c: 1.23 }, { c: 89.0000001 }],
+      height: 500,
+    };
+    render(<FilterableTable {...floatProps} />);
+
+    const floatColumn = screen.getByRole('columnheader', { name: 'c' });
+    const gridCells = screen.getAllByRole('gridcell');
+
+    // Original order
+    expect(gridCells[0]).toHaveTextContent('45.67');
+    expect(gridCells[1]).toHaveTextContent('1.23');
+    expect(gridCells[2]).toHaveTextContent('89.0000001');
+
+    // First click to sort ascending
+    userEvent.click(floatColumn);
+    expect(gridCells[0]).toHaveTextContent('1.23');
+    expect(gridCells[1]).toHaveTextContent('45.67');
+    expect(gridCells[2]).toHaveTextContent('89.0000001');
+
+    // Second click to sort descending
+    userEvent.click(floatColumn);
+    expect(gridCells[0]).toHaveTextContent('89.0000001');
+    expect(gridCells[1]).toHaveTextContent('45.67');
+    expect(gridCells[2]).toHaveTextContent('1.23');
+
+    // Third click to sort ascending again
+    userEvent.click(floatColumn);
+    expect(gridCells[0]).toHaveTextContent('1.23');
+    expect(gridCells[1]).toHaveTextContent('45.67');
+    expect(gridCells[2]).toHaveTextContent('89.0000001');
+  });
+
+  it('sorts rows properly when floating numbers have mixed types', () => {
+    const mixedFloatProps = {
+      orderedColumnKeys: ['d'],
+      data: [
+        { d: 48710.92 },
+        { d: 145776.56 },
+        { d: 72212.86 },
+        { d: '144729.96000000002' },
+        { d: '26260.210000000003' },
+        { d: '152718.97999999998' },
+        { d: 28550.59 },
+        { d: '24078.610000000004' },
+        { d: '98089.08000000002' },
+        { d: '3439718.0300000007' },
+        { d: '4528047.219999993' },
+      ],
+      height: 500,
+    };
+    render(<FilterableTable {...mixedFloatProps} />);
+
+    const mixedFloatColumn = screen.getByRole('columnheader', { name: 'd' });
+    const gridCells = screen.getAllByRole('gridcell');
+
+    // Original order
+    expect(gridCells[0]).toHaveTextContent('48710.92');
+    expect(gridCells[1]).toHaveTextContent('145776.56');
+    expect(gridCells[2]).toHaveTextContent('72212.86');
+    expect(gridCells[3]).toHaveTextContent('144729.96000000002');
+    expect(gridCells[4]).toHaveTextContent('26260.210000000003');
+    expect(gridCells[5]).toHaveTextContent('152718.97999999998');
+    expect(gridCells[6]).toHaveTextContent('28550.59');
+    expect(gridCells[7]).toHaveTextContent('24078.610000000004');
+    expect(gridCells[8]).toHaveTextContent('98089.08000000002');
+    expect(gridCells[9]).toHaveTextContent('3439718.0300000007');
+    expect(gridCells[10]).toHaveTextContent('4528047.219999993');
+
+    // First click to sort ascending
+    userEvent.click(mixedFloatColumn);
+    expect(gridCells[0]).toHaveTextContent('24078.610000000004');
+    expect(gridCells[1]).toHaveTextContent('26260.210000000003');
+    expect(gridCells[2]).toHaveTextContent('28550.59');
+    expect(gridCells[3]).toHaveTextContent('48710.92');
+    expect(gridCells[4]).toHaveTextContent('72212.86');
+    expect(gridCells[5]).toHaveTextContent('98089.08000000002');
+    expect(gridCells[6]).toHaveTextContent('144729.96000000002');
+    expect(gridCells[7]).toHaveTextContent('145776.56');
+    expect(gridCells[8]).toHaveTextContent('152718.97999999998');
+    expect(gridCells[9]).toHaveTextContent('3439718.0300000007');
+    expect(gridCells[10]).toHaveTextContent('4528047.219999993');
+
+    // Second click to sort descending
+    userEvent.click(mixedFloatColumn);
+    expect(gridCells[0]).toHaveTextContent('4528047.219999993');
+    expect(gridCells[1]).toHaveTextContent('3439718.0300000007');
+    expect(gridCells[2]).toHaveTextContent('152718.97999999998');
+    expect(gridCells[3]).toHaveTextContent('145776.56');
+    expect(gridCells[4]).toHaveTextContent('144729.96000000002');
+    expect(gridCells[5]).toHaveTextContent('98089.08000000002');
+    expect(gridCells[6]).toHaveTextContent('72212.86');
+    expect(gridCells[7]).toHaveTextContent('48710.92');
+    expect(gridCells[8]).toHaveTextContent('28550.59');
+    expect(gridCells[9]).toHaveTextContent('26260.210000000003');
+    expect(gridCells[10]).toHaveTextContent('24078.610000000004');
+
+    // Third click to sort ascending again
+    userEvent.click(mixedFloatColumn);
+    expect(gridCells[0]).toHaveTextContent('24078.610000000004');
+    expect(gridCells[1]).toHaveTextContent('26260.210000000003');
+    expect(gridCells[2]).toHaveTextContent('28550.59');
+    expect(gridCells[3]).toHaveTextContent('48710.92');
+    expect(gridCells[4]).toHaveTextContent('72212.86');
+    expect(gridCells[5]).toHaveTextContent('98089.08000000002');
+    expect(gridCells[6]).toHaveTextContent('144729.96000000002');
+    expect(gridCells[7]).toHaveTextContent('145776.56');
+    expect(gridCells[8]).toHaveTextContent('152718.97999999998');
+    expect(gridCells[9]).toHaveTextContent('3439718.0300000007');
+    expect(gridCells[10]).toHaveTextContent('4528047.219999993');
+  });
+});
diff --git a/superset-frontend/src/components/FilterableTable/FilterableTable.tsx b/superset-frontend/src/components/FilterableTable/FilterableTable.tsx
@@ -322,21 +322,30 @@ 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;
+  };
+
   sortResults(sortBy: string, descending: boolean) {
     return (a: Datum, b: Datum) => {
-      const aValue = a[sortBy];
-      const bValue = b[sortBy];
+      const aValue = this.parseFloatingNums(a[sortBy]);
+      const bValue = this.parseFloatingNums(b[sortBy]);
+
+      // equal items sort equally
       if (aValue === bValue) {
-        // equal items sort equally
         return 0;
       }
+
+      // nulls sort after anything else
       if (aValue === null) {
-        // nulls sort after anything else
         return 1;
       }
       if (bValue === null) {
         return -1;
       }
+
       if (descending) {
         return aValue < bValue ? 1 : -1;
       }