[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17573: fix(sqllab): Floating numbers not sorting correctly in result column

[GitBox <gi...@apache.org>]

lyndsiWilliams commented on a change in pull request #17573:
URL: https://github.com/apache/superset/pull/17573#discussion_r760674672



##########
File path: superset-frontend/src/components/FilterableTable/FilterableTable.tsx
##########
@@ -323,19 +323,19 @@ export default class FilterableTable extends PureComponent<
 
   sortResults(sortBy: string, descending: boolean) {
     return (a: Datum, b: Datum) => {
-      const aValue = a[sortBy];
-      const bValue = b[sortBy];
-      if (aValue === bValue) {
-        // equal items sort equally
-        return 0;
-      }
-      if (aValue === null) {
-        // nulls sort after anything else
-        return 1;
-      }
-      if (bValue === null) {
-        return -1;
-      }
+      // Parse any floating numbers so they'll sort correctly
+      const parseFloatingNums = (value: any) => {
+        const floatValue = parseFloat(value);
+        return Number.isNaN(floatValue) ? value : parseFloat(value);
+      };
+
+      const aValue = parseFloatingNums(a[sortBy]);
+      const bValue = parseFloatingNums(b[sortBy]);
+
+      String(aValue).localeCompare(String(bValue), undefined, {

Review comment:
       I did a lot of playing around with this and figured out that the sorting works without `localeCompare` so I removed it and added testing in [`this commit`](https://github.com/apache/superset/pull/17573/commits/8be6346d756a43203abf820c9e88d9c2f8f63b13). While testing I found any floating point number that had more than 2 floating points (like `12.001` or longer) is a `string`, while any other number is a `number`. I think originally, the floating numbers weren't sorting correctly because the types were mixed. Just using the `parseFloatingNums` function seems to solve this so maybe we can look at using `localeCompare` in the future for multilingual sorting, since it just seems to be causing issues in this instance. I also didn't realize you had to explicitly pass the language you want to support as a second argument, so I don't think I'll be able to make this work with `localeCompare`.




-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org