fix type coercion in bmerge #6603
Conversation
|
Generated via commit 91c53c5 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 #6603 +/- ##
=======================================
Coverage 98.60% 98.61%
=======================================
Files 79 79
Lines 14516 14547 +31
=======================================
+ Hits 14314 14345 +31
Misses 202 202 ☔ View full report in Codecov by Sentry. |
|
@MichaelChirico is it worth to make the (length(unique(icols))!=length(icols) || length(unique(xcols))!=length(xcols))ensuring that either |
|
Sorry, I just saw that this is a CRAN requirement, checking now. |
R/bmerge.R
Outdated
| @@ -71,7 +71,13 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos | |||
| stopf("Incompatible join types: %s (%s) and %s (%s). Factor columns must join to factor or character columns.", xname, xclass, iname, iclass) | |||
| } | |||
| if (xclass == iclass) { | |||
There was a problem hiding this comment.
aside: {x,i}class seem like poor name choices here, given that they typically come from typeof() (and class() is never called, except maybe internally by inherits()).
{x,i}type would be more appropriate, but {x,i}_merge_type might be even better?
There was a problem hiding this comment.
another aside: It looks like we do x[[xc]] and i[[ic]] extraction at least one and up to several times. Should we just store the extracted column locally?
There was a problem hiding this comment.
aside:
{x,i}classseem like poor name choices here, given that they typically come fromtypeof()(andclass()is never called, except maybe internally byinherits()).
Yes but didn't want to change more than needed for the quick fix
|
Definitely thanks for triaging a fix here. I think we both agree it's pretty hack-y & ideally not needed. I'm not sure I fully understand the bug yet, but as stated in OP we're doing IMO |
Unfortunately this "contract" does not hold. I have a Windows dev version installed here and get the following > typeof(.Date(0L))
[1] "integer"
> typeof(as.Date(.Date(0L)))
[1] "integer" |
Thanks, also confirmed that on 4.4.1 |
|
This is strange as Kurt mentioned there wasn't an intention to do this but I figure it would have been fixed by now if that were the case. |
|
Update this fix now can convert into one direction from integer to double # this works
x = data.table(a=1L)
y = data.table(c=1L, d=2)
y[x, on=.(c==a, d==a)]
y[x, on=.(d==a, c==a)]
# this still needs to fixed
x = data.table(a=1)
y = data.table(c=1, d=2L)
y[x, on=.(c==a, d==a)]
y[x, on=.(d==a, c==a)] |
Should I consider this PR in-progress for now? The diff has grown enough that it would help to add a brief overview of the changes to the PR description to orient reading |
R/bmerge.R
Outdated
| newvalue = chmatch(i[[ic]], levels(x[[xc]]), nomatch=0L) | ||
| if (anyNA(i[[ic]])) newvalue[is.na(i[[ic]])] = NA_integer_ # NA_character_ should match to NA in factor, #3809 | ||
| set(i, j=ic, value=newvalue) | ||
| if (nrow(i)) { |
There was a problem hiding this comment.
Haven't read carefully yet, but given the description of the problem as I understand it now, I had expected something like this to be needed:
(1) A pass over the join columns to determine the "join type" of each column
(2) A second pass to actually do the coercions as needed
Does that make sense to you? If so, each step could be a helper function which would improve readability & modularity.
There was a problem hiding this comment.
In theory yes, but we actually only have to take a look the case of mixing double and integer columns, since this is the only case where we previously made bmerge smart enough to cater for different types (besides character and factors)
|
Just received the "deadline" on this from Kurt:
|
Closes #6602
NB: I'm not happy that a user can get different messages, depending on the number of join conditions, but coercing both Dates to
doublewhen multiple column conditions exist, seems like the right decision.Base does not encounter this problem since one join column can not be in multiple join conditions.