Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Dec 4, 2021

Conversation

lyndsiWilliams
Copy link
Member

@lyndsiWilliams lyndsiWilliams commented Nov 29, 2021

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

floatSortBefore

AFTER

floatSortAfter

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

@lyndsiWilliams 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
@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

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

Impacted file tree graph

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, it's Number.isNan 😅

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const aValue = parseFloatingNums(a[sortBy]);
const bValue = parseFloatingNums(b[sortBy]);

String(aValue).localeCompare(String(bValue), undefined, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@betodealmeida betodealmeida Nov 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@lyndsiWilliams can you also write a test for this?

@lyndsiWilliams
Copy link
Member Author

@lyndsiWilliams can you also write a test for this?

You got it! 😁

@pull-request-size pull-request-size bot added size/L and removed size/S labels Dec 2, 2021
// Parse any floating numbers so they'll sort correctly
parseFloatingNums = (value: any) => {
const floatValue = parseFloat(value);
return Number.isNaN(floatValue) ? value : parseFloat(value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

@eschutho eschutho merged commit 05752e3 into apache:master Dec 4, 2021
@eschutho eschutho deleted the lyndsi/fix-float-column-sort branch December 4, 2021 00:31
eschutho pushed a commit that referenced this pull request Jan 27, 2022
…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
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/L v1.4.1 🍒 1.4.1 🍒 1.4.2 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants