fix incorrect key when one and only one NA is at the beginning #3443
Conversation
R/data.table.R
Outdated
| if (keylen && (ichk || is.logical(i) || (.Call(CisOrderedSubset, irows, nrow(x)) && ((roll == FALSE) || length(irows) == 1L)))) # see #1010. don't set key when i has no key, but irows is ordered and roll != FALSE | ||
| if (keylen && ( | ||
| ichk || is.logical(i) || ( | ||
| .Call(CisOrderedSubset, irows, nrow(x)) && !missingroll && ( # !missingroll fix for #3441 |
There was a problem hiding this comment.
might not be clear due to reformatting... only !missingroll has been added here
jangorecki
changed the title
| @@ -1248,17 +1248,21 @@ SEXP isOrderedSubset(SEXP x, SEXP nrow) | |||
| // specialized for use in [.data.table only | |||
| // Ignores 0s but heeds NAs and any out-of-range (which result in NA) | |||
There was a problem hiding this comment.
what was designed behaviour for NA based on that comment? it it will return FALSE for any NA, which solves the issue, and does not break any tests
There was a problem hiding this comment.
The behaviour intended was to heed NAs; meaning NAs anywhere should return FALSE. It was getting it wrong then when there is one NA at the beginning. Good spot and fix.
jangorecki
changed the title
Codecov Report
@@ Coverage Diff @@
## master #3443 +/- ##
==========================================
+ Coverage 95.11% 95.13% +0.02%
==========================================
Files 65 65
Lines 12310 12303 -7
==========================================
- Hits 11709 11705 -4
+ Misses 601 598 -3
Continue to review full report at Codecov.
|
mattdowle
changed the title
closes #3441.
Adds some overhead to C
isOrderedSubsetfunction which is called in few places, so best to confirm there is no faster way. And that it is the proper way to solve that at all.