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

lyndsiWilliams · 2021-11-29T14:01:27Z

SUMMARY

Floating numbers were not sorting correctly in the result column. This was fixed by creating a parseFloatingNums function to parse any numbers in the result set.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

AFTER

TESTING INSTRUCTIONS

Go to SQL Lab
Enter the following query using the superset example database:

SELECT status, year, sum(sales)
FROM main.cleaned_sales_data
GROUP BY status, year

Click the sales column to sort
Observe that floating numbers are now sorting correctly

ADDITIONAL INFORMATION

Has associated issue: [sql_lab] Sqllab results not sorting float column correctly #16971
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2021-11-29T14:43:41Z

Codecov Report

Merging #17573 (d1d0751) into master (d05c561) will decrease coverage by 8.41%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #17573      +/-   ##
==========================================
- Coverage   76.99%   68.58%   -8.42%     
==========================================
  Files        1047     1589     +542     
  Lines       56497    64959    +8462     
  Branches     7799     6993     -806     
==========================================
+ Hits        43499    44549    +1050     
- Misses      12742    18526    +5784     
- Partials      256     1884    +1628

Flag	Coverage Δ
javascript	`57.13% <100.00%> (-13.92%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...src/components/FilterableTable/FilterableTable.tsx	`72.31% <100.00%> (-10.04%)`	⬇️
...nd/src/explore/components/DatasourcePanel/types.ts	`33.33% <0.00%> (-66.67%)`	⬇️
...eModal/DatabaseConnectionForm/CommonParameters.tsx	`51.42% <0.00%> (-43.02%)`	⬇️
...RUD/data/database/DatabaseModal/SqlAlchemyForm.tsx	`58.33% <0.00%> (-33.98%)`	⬇️
...set-frontend/src/views/CRUD/data/database/state.ts	`66.66% <0.00%> (-33.34%)`	⬇️
...ntend/src/dashboard/containers/DashboardHeader.jsx	`66.66% <0.00%> (-33.34%)`	⬇️
...set-frontend/src/dashboard/reducers/datasources.ts	`42.85% <0.00%> (-32.15%)`	⬇️
superset-frontend/src/components/Select/utils.ts	`63.63% <0.00%> (-28.68%)`	⬇️
.../CRUD/data/database/DatabaseModal/ExtraOptions.tsx	`65.85% <0.00%> (-27.33%)`	⬇️
...t-frontend/src/components/PopoverSection/index.tsx	`73.33% <0.00%> (-26.67%)`	⬇️
... and 1077 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d05c561...d1d0751. Read the comment docs.

AAfghahi · 2021-11-29T15:57:39Z

superset-frontend/src/components/FilterableTable/FilterableTable.tsx

+
+      // Parse any floating numbers so they'll sort correctly
+      const parseFloatingNums = (value: any) =>
+        value.isNan ? value : parseFloat(value);


that's cool, didn't know that isNan was a function!

Oops, it's Number.isNan 😅

betodealmeida

I think this will fail if the values are not numbers/NaN, can you check?

superset-frontend/src/components/FilterableTable/FilterableTable.tsx

eschutho · 2021-11-30T22:15:12Z

superset-frontend/src/components/FilterableTable/FilterableTable.tsx

+      const aValue = parseFloatingNums(a[sortBy]);
+      const bValue = parseFloatingNums(b[sortBy]);
+
+      String(aValue).localeCompare(String(bValue), undefined, {


if you're using localeCompare with the numeric option, then do you still need to convert to a float? It looks like you're converting to a float and then back again to a string? It seems redundant.

Also, nice find on localeCompare, but I don't think it's going to support multilingually unless you explicitly pass the language that you want to support as the second argument.

I need to parse the floating numbers for them to sort correctly, but localeCompare will only take strings. The numerical option allows it to check the strings for numbers.

@lyndsiWilliams but when you do String(aValue) and String(bValue) you're converting them back to strings. It should work if you just call localeCompare on the values without parseFloat, assuming you pass the numeric option:

"20.0".localeCompare("10", undefined, {numeric: true}) 1 "2.0".localeCompare("10", undefined, {numeric: true}) -1

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. 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.

eschutho · 2021-11-30T23:01:49Z

@lyndsiWilliams can you also write a test for this?

lyndsiWilliams · 2021-11-30T23:05:00Z

@lyndsiWilliams can you also write a test for this?

You got it! 😁

betodealmeida · 2021-12-02T01:29:02Z

superset-frontend/src/components/FilterableTable/FilterableTable.tsx

+  // Parse any floating numbers so they'll sort correctly
+  parseFloatingNums = (value: any) => {
+    const floatValue = parseFloat(value);
+    return Number.isNaN(floatValue) ? value : parseFloat(value);


Nit:

Suggested change

return Number.isNaN(floatValue) ? value : parseFloat(value);

return Number.isNaN(floatValue) ? value : floatValue

Whoops, I missed that one! Thank you, fixed in this commit 😁

eschutho

looks great!

…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

Floating nums now sorting correctly with parseFloatingNums function

a57cf70

superset-github-bot bot added the preset-io label Nov 29, 2021

pull-request-size bot added the size/XS label Nov 29, 2021

lyndsiWilliams changed the title ~~fix (sqllab): Floating numbers not sorting correctly in result column~~ fix(sqllab): Floating numbers not sorting correctly in result column Nov 29, 2021

AAfghahi reviewed Nov 29, 2021

View reviewed changes

betodealmeida requested changes Nov 29, 2021

View reviewed changes

superset-frontend/src/components/FilterableTable/FilterableTable.tsx Outdated Show resolved Hide resolved

Floating numbers AND strings now sorting correctly, +locale comparison

43742de

pull-request-size bot added size/S and removed size/XS labels Nov 30, 2021

betodealmeida requested changes Nov 30, 2021

View reviewed changes

superset-frontend/src/components/FilterableTable/FilterableTable.tsx Outdated Show resolved Hide resolved

superset-frontend/src/components/FilterableTable/FilterableTable.tsx Outdated Show resolved Hide resolved

eschutho reviewed Nov 30, 2021

View reviewed changes

lyndsiWilliams added 2 commits November 30, 2021 16:21

Added NULL handling back to sort function

580f9d9

Moved parseFloatingNums outside of sortResults

628a549

Removed localeCompare and added testing

8be6346

pull-request-size bot added size/L and removed size/S labels Dec 2, 2021

Add equality check back to sort function

4904ef1

betodealmeida approved these changes Dec 2, 2021

View reviewed changes

Added floatValue nit

d1d0751

eschutho approved these changes Dec 2, 2021

View reviewed changes

eschutho merged commit 05752e3 into apache:master Dec 4, 2021

eschutho deleted the lyndsi/fix-float-column-sort branch December 4, 2021 00:31

etr2460 mentioned this pull request Jan 11, 2022

fix: SQL Lab sorting of non-numbers #18006

Merged

9 tasks

eschutho added the v1.4.1 label Jan 24, 2022

mistercrunch added 🍒 1.4.1 🍒 1.4.2 labels Mar 13, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lyndsiWilliams commented Nov 29, 2021 •

edited

codecov bot commented Nov 29, 2021 •

edited

AAfghahi Nov 29, 2021

lyndsiWilliams Nov 30, 2021

betodealmeida left a comment

eschutho Nov 30, 2021

eschutho Nov 30, 2021

lyndsiWilliams Nov 30, 2021

betodealmeida Nov 30, 2021 •

edited

lyndsiWilliams Dec 2, 2021

eschutho commented Nov 30, 2021

lyndsiWilliams commented Nov 30, 2021

betodealmeida Dec 2, 2021

lyndsiWilliams Dec 2, 2021

eschutho left a comment

	return Number.isNaN(floatValue) ? value : parseFloat(value);
	return Number.isNaN(floatValue) ? value : floatValue

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

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

Conversation

lyndsiWilliams commented Nov 29, 2021 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Nov 29, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida Nov 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho commented Nov 30, 2021

lyndsiWilliams commented Nov 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

lyndsiWilliams commented Nov 29, 2021 •

edited

codecov bot commented Nov 29, 2021 •

edited

betodealmeida Nov 30, 2021 •

edited