Rbind allow binding of different class attributes #5446
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5446 +/- ##
=======================================
Coverage 97.53% 97.53%
=======================================
Files 80 80
Lines 14915 14926 +11
=======================================
+ Hits 14547 14558 +11
Misses 368 368 ☔ View full report in Codecov by Sentry. |
R/merge.R
Outdated
| @@ -97,7 +97,7 @@ merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FAL | |||
| # Perhaps not very commonly used, so not a huge deal that the join is redone here. | |||
| missingyidx = y[!x, which=TRUE, on=by, allow.cartesian=allow.cartesian] | |||
| if (length(missingyidx)) { | |||
| dt = rbind(dt, y[missingyidx], use.names=FALSE, fill=TRUE) | |||
| dt = rbind(dt, y[missingyidx], use.names=FALSE, fill=TRUE, ignore.attr=TRUE) | |||
There was a problem hiding this comment.
Do we have unit test that covers that? It looks like a breaking change, therefore should be introduced softly, having backward compatible default for 1-2 versions, before switching to new behavior.
There was a problem hiding this comment.
We have now since #5857. I'm indifferent whether we add the change to merge or not. It makes the code much shorter and does not hide so much what we want to achieve but since many ppl rely on merge we should be careful about changes.
|
This PR resolves one of the issues on 1.15.0 milestone. But it is much bigger than just fixing regression, it adds new feature. I would therefore prefer to shift it to 1.15.99, and for 1.15.0 push only regression fixes. |
|
Agree with Jan, it would be nice to merge a minimal PR that fixes regression to send out as 1.15.0. I glanced at the diff here and didn't see an easy way to separate out the regression fix from the new functionality, is that possible @ben-schwen? Or should we work on that as a standalone PR? |
|
#5309 has two issues in it:
Warning message:
In rbindlist(l, use.names, fill, idcol) :
use.names= cannot be FALSE when fill is TRUE. Setting use.names=TRUE.This error of #5309 is already fixed by #5468
Will add tests of #5263 and file regression PR. |
|
All LGTM, minor requests for further improvement. Defer to @jangorecki about |
|
Probably full outer join do rbind |
Co-authored-by: Michael Chirico <chiricom@google.com>
…to rbind_class_att
Closes #5309 (shadowing current approach for
int64andfactor)Closes #3911 (automatically allows for mixing
DateandIDateand addsignore.attrargument)Closes #4934 (only carries class of
AsIsto result ifAsIsis first in binding to stay conform withdo.call(rbind, list)in this case)Closes #5391
Closes #5542
Towards #5486 also needs #5569
DateandIDate) withatomiccolumnsPOSIXctwithatomiccolumnsITimeAsIsignore.attrargumentrbindlistandrbindto manually deactivate check for equal classes of binding columns