Allow double-integer64 joins when double is in (integer32 , integer64] range #6626
Conversation
MichaelChirico
requested review from
ben-schwen and
jangorecki
as code owners
|
Generated via commit 952de9e Download link for the artifact containing the test results: ↓ atime-results.zip
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6626 +/- ##
=======================================
Coverage 98.61% 98.61%
=======================================
Files 79 79
Lines 14533 14543 +10
=======================================
+ Hits 14332 14342 +10
Misses 201 201 ☔ View full report in Codecov by Sentry. |
src/utils.c
Outdated
| static R_xlen_t firstNonInt(SEXP x) { | ||
| // used to error if not passed type double but this needed extra is.double() calls in calling R code | ||
| // which needed a repeat of the argument. Hence simpler and more robust to return false when not type double. | ||
| bool isRealReallyInt32(SEXP x) { |
There was a problem hiding this comment.
maybe isRealAsInt32 and isRealAsInt64 or RealIsInt32 and RealIsInt64?
There was a problem hiding this comment.
that sounds like the function does coercion to me.
Maybe isDoubleAndInt32? To convey it's in the intersection of the two storage types?
If not that, let's earmark this for discussion in a follow-up issue, since I just adapted the existing naming isRealReallyInt() to be more specific in this PR.
There was a problem hiding this comment.
isDoubleAndInt32 would work for me, maybe even isRealAndInt32 to convey the difference between R data types and underlying C data types.
There was a problem hiding this comment.
Hmm I'm rethinking. That naming implies a symmetry, e.g. suggesting it could accept integer inputs also.
WDYT about isInt32AsReal? realFitsInInt32/fitsInInt32? That highlights we are only interested in whether something currently stored as double could be stored as int instead.
(for the int (32) case, the symmetry part is kinda trivial, because int is a strict subset of double, but the symmetry does not apply to the int64_t case, hence why I'm hesitating)
|
LGTM, besides that I find the current naming of |
|
I'll go ahead and merge since the routine name is just a minor detail. We can easily file a follow-up PR to fine-tune. |
Closes #4167.
Probably has some conflict with #6602 -- that one is the priority since it's a CRAN blocker.
I decided to totally ditch
isReallyReal()-- it's only ever used inif()statements, so the feature to return the first true-real index is not useful. That leads to the IMO much more readableisRealReallyInt{32,64}approach here.