You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/01/11 21:51:23 UTC
[GitHub] [superset] etr2460 opened a new pull request #18006: fix: SQL Lab sorting of non-numbers
etr2460 opened a new pull request #18006:
URL: https://github.com/apache/superset/pull/18006
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
https://github.com/apache/superset/pull/17573 fixed the sorting of floating point numbers, but broke sorting strings that start with numbers. The most common of these are dses like `2022-01-01`, and `parseFloat("2022-01-01") === 2022`, so sorting got all janky.
I've fixed this by ensuring that we only try to parse numbers if they pass a regex that matches JS's format of numbers.
Note that this solution isn't 100% accurate, and will still cause undefined behavior when sorting a list containing both numeric and non numeric strings. I don't have a good answer for fixing this, but perhaps in the future we'll need to refactor this further. I'd imagine a per column parameter that defines the sorting function (convert to number and then sort, do string sort, sort in hexadecimal because all the strings start with `0x` and contain only `0-f`, something else) but that was very much out of scope of this bug fix.
### TESTING INSTRUCTIONS
New unit test (and ensure the unit test added in #17573 still works)
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
to: @lyndsiWilliams @eschutho @graceguo-supercat @ktmud
--
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
[GitHub] [superset] etr2460 commented on pull request #18006: fix: SQL Lab sorting of non-numbers
Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #18006:
URL: https://github.com/apache/superset/pull/18006#issuecomment-1011315590
/testenv up
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #18006: fix: SQL Lab sorting of non-numbers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18006:
URL: https://github.com/apache/superset/pull/18006#issuecomment-1011365978
Ephemeral environment shutdown and build artifacts deleted.
--
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
[GitHub] [superset] etr2460 merged pull request #18006: fix: SQL Lab sorting of non-numbers
Posted by GitBox <gi...@apache.org>.
etr2460 merged pull request #18006:
URL: https://github.com/apache/superset/pull/18006
--
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
[GitHub] [superset] ktmud commented on a change in pull request #18006: fix: SQL Lab sorting of non-numbers
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #18006:
URL: https://github.com/apache/superset/pull/18006#discussion_r782620360
##########
File path: superset-frontend/src/components/FilterableTable/FilterableTable.tsx
##########
@@ -322,16 +322,26 @@ 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') {
+ // 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 onlyNumberRegEx =
+ /^(NaN|-?((\d*\.\d+|\d+)([Ee][+-]?\d+)?|Infinity))$/;
Review comment:
Maybe move this to a global constant or create a helper function?
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #18006: fix: SQL Lab sorting of non-numbers
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18006:
URL: https://github.com/apache/superset/pull/18006#issuecomment-1010464630
# [Codecov](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18006](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2159734) into [master](https://codecov.io/gh/apache/superset/commit/bdc35a221445d9ba62a4cfabc2f5561dc712084c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bdc35a2) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18006 +/- ##
=======================================
Coverage 66.32% 66.32%
=======================================
Files 1569 1569
Lines 61658 61661 +3
Branches 6232 6233 +1
=======================================
+ Hits 40892 40895 +3
Misses 19167 19167
Partials 1599 1599
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `50.87% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...src/components/FilterableTable/FilterableTable.tsx](https://codecov.io/gh/apache/superset/pull/18006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRmlsdGVyYWJsZVRhYmxlL0ZpbHRlcmFibGVUYWJsZS50c3g=) | `72.02% <100.00%> (+0.50%)` | :arrow_up: |
| [...s/legacy-plugin-chart-country-map/src/countries.ts](https://codecov.io/gh/apache/superset/pull/18006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNvdW50cnktbWFwL3NyYy9jb3VudHJpZXMudHM=) | `100.00% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [bdc35a2...2159734](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #18006: fix: SQL Lab sorting of non-numbers
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18006:
URL: https://github.com/apache/superset/pull/18006#issuecomment-1010464630
# [Codecov](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18006](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4fb1780) into [master](https://codecov.io/gh/apache/superset/commit/bdc35a221445d9ba62a4cfabc2f5561dc712084c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bdc35a2) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 4fb1780 differs from pull request most recent head 2159734. Consider uploading reports for the commit 2159734 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18006 +/- ##
=======================================
Coverage 66.32% 66.32%
=======================================
Files 1569 1569
Lines 61658 61661 +3
Branches 6232 6233 +1
=======================================
+ Hits 40892 40895 +3
Misses 19167 19167
Partials 1599 1599
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `50.87% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...src/components/FilterableTable/FilterableTable.tsx](https://codecov.io/gh/apache/superset/pull/18006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRmlsdGVyYWJsZVRhYmxlL0ZpbHRlcmFibGVUYWJsZS50c3g=) | `72.02% <100.00%> (+0.50%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [bdc35a2...2159734](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #18006: fix: SQL Lab sorting of non-numbers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18006:
URL: https://github.com/apache/superset/pull/18006#issuecomment-1011317687
@etr2460 Ephemeral environment spinning up at http://54.69.38.190:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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
[GitHub] [superset] codecov[bot] commented on pull request #18006: fix: SQL Lab sorting of non-numbers
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #18006:
URL: https://github.com/apache/superset/pull/18006#issuecomment-1010464630
# [Codecov](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18006](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4fb1780) into [master](https://codecov.io/gh/apache/superset/commit/bdc35a221445d9ba62a4cfabc2f5561dc712084c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bdc35a2) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18006 +/- ##
=======================================
Coverage 66.32% 66.32%
=======================================
Files 1569 1569
Lines 61658 61661 +3
Branches 6232 6233 +1
=======================================
+ Hits 40892 40895 +3
Misses 19167 19167
Partials 1599 1599
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `50.87% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...src/components/FilterableTable/FilterableTable.tsx](https://codecov.io/gh/apache/superset/pull/18006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRmlsdGVyYWJsZVRhYmxlL0ZpbHRlcmFibGVUYWJsZS50c3g=) | `72.02% <100.00%> (+0.50%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [bdc35a2...4fb1780](https://codecov.io/gh/apache/superset/pull/18006?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] etr2460 commented on pull request #18006: fix: SQL Lab sorting of non-numbers
Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #18006:
URL: https://github.com/apache/superset/pull/18006#issuecomment-1011333785
verified that 10k rows still sort pretty much instantly, both for numeric and non numeric columns
--
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
[GitHub] [superset] etr2460 commented on pull request #18006: fix: SQL Lab sorting of non-numbers
Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #18006:
URL: https://github.com/apache/superset/pull/18006#issuecomment-1010529031
>Did you check whether the performance is acceptable when table has the maximum number of rows? I'd imagine regex match for every row to be quite expensive. In regards to the RegEx used, it's probably more important what the database returns other than what JavaScript accepts. For example, some database may return Infinity as Inf.
@ktmud I did not check yet, although aren't regexes pretty fast usually? I suppose i could do a faster check first before the more expensive regex, but my thought was that the `parseFloat` was already taking a while (that we run on each iteration) so maybe it didn't matter. Regardless I'll test a large result set and report back.
As for matching what databases may return, I suppose i could try to handle everything every database returns, but i figured having something consistent with a single language would be worthwhile. If this was breaking for you with a specific database, then you could always write `if(column_name = Inf, 'Infinity', column_name) as column_name` in your query, while if we don't have this handling, it could never sort properly (or only would if you did something like `if(column_name = Inf, 99999999999999999999, column_name) as column_name`
So in summary, i'll test perf and if that looks good, will merge (with the moving of the regex to a global constant, sorry for not doing that in the first place)
--
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