Addressing transform sort order with null values#4783
Conversation
| } else if(b.v === null) { | ||
| return 1; | ||
| } | ||
| return d2c(a.v) - d2c(b.v); |
There was a problem hiding this comment.
Thanks for the PR @oshikryu - looking good, and your test additions are excellent!
I think we want to treat any sort of invalid data the same as null - which, after running through d2c, becomes BADNUM from src/constants/numerical. BADNUM is just undefined but for clarity and consistency we prefer to use the named constant.
So this would look something like:
var ac = d2c(a.v);
var bc = d2c(b.v);
if(ac === BADNUM) {
return -1;
}
if(bc === BADNUM) {
return 1;
}
return ac - bc;Also I suppose this can be up for discussion, but in several other contexts (eg Dash DataTable) we sort null values always at the end - the idea being you want them out of the way where you mostly ignore them. Here you're sorting them as "extra negative", which puts them last when sorting descending, but FIRST when sorting ascending.
There was a problem hiding this comment.
Good point about "extra negative" for BADNUM values. I added the reference to BADNUM in the sort and updated the tests
Changing sort transform tests to include check for BADNUM Sort ascending has BADNUM last (not "extra negative" where BADNUM is first) Sort descending has BADNUM last
|
I'm not really sure what downstream errors were produced with this change. I could use some guidance @alexcjohnson |
alexcjohnson
left a comment
alexcjohnson
left a comment
There was a problem hiding this comment.
Excellent, thanks for the change, this looks good to me! 💃 @archmoj please merge when you're ready.
I'm not really sure what downstream errors were produced with this change.
Were the tests failing before your force push? Don't worry about it, the tests have been a little bit flaky lately and sometimes need to be rerun. For future reference though we discourage force pushes in this repo once a PR is open, as it can confuse the review process.
3 participants
Updating sort function to handle null value case
Changing sort transform tests to include null value
This should fix the issue in #4784
Example working codepen:
https://codepen.io/oshikryu/pen/rNOjeZb