Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove use of rbindlist(..., use.names=FALSE, fill=TRUE) in merge #5857

Merged
merged 4 commits into from
Dec 27, 2023

Conversation

ben-schwen
Copy link
Member

@ben-schwen ben-schwen commented Dec 26, 2023

Closes #5309 (error part in rbind is duplicate of #5444)
Pre #5446

Copy link

codecov bot commented Dec 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (78dee17) 97.47% compared to head (123ac11) 97.47%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5857   +/-   ##
=======================================
  Coverage   97.47%   97.47%           
=======================================
  Files          80       80           
  Lines       14823    14829    +6     
=======================================
+ Hits        14448    14454    +6     
  Misses        375      375           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MichaelChirico
Copy link
Member

I would add this example from #5309 as well -- surprising for the PR closing an issue on rbind() to only include tests on merge():

t2 = data.table(a = NA_real_, b = NA_real_, c = NA_real_, d= as.Date(NA))
t1 = data.table(a = 1.1, b = 1.1, c = 1.1, d = as.Date('2021-10-05'), e = as.POSIXct("2021-10-06 13:58:00 UTC"))
r2 = rbind(t1,t2, fill = T, use.names = F)

Or maybe:

error part in rbind is duplicate of #5444

Implies that a test already exists? If so please help point to it. But #5444 summary mentions R crashing, and #5309 was a normal R error, so I'm still not sure of the connection.

@ben-schwen
Copy link
Member Author

I would add this example from #5309 as well -- surprising for the PR closing an issue on rbind() to only include tests on merge():

t2 = data.table(a = NA_real_, b = NA_real_, c = NA_real_, d= as.Date(NA))
t1 = data.table(a = 1.1, b = 1.1, c = 1.1, d = as.Date('2021-10-05'), e = as.POSIXct("2021-10-06 13:58:00 UTC"))
r2 = rbind(t1,t2, fill = T, use.names = F)

Or maybe:

error part in rbind is duplicate of #5444

Implies that a test already exists? If so please help point to it. But #5444 summary mentions R crashing, and #5309 was a normal R error, so I'm still not sure of the connection.

Added the test.

Problem on both #5309 and #5444 was that non-existing columns were compared to existing columns due to fill. In #5444 this even led to a segfault (getAttrib on non-existing list column) while in #5309 it said to different attributes (getAttrib on non-existing data.table column)

@ben-schwen ben-schwen changed the title remove use of rbindlist(..., use.names=FALSE, fill=TRUE) in merge fix use of rbindlist(..., use.names=FALSE, fill=TRUE) in merge Dec 27, 2023
@ben-schwen ben-schwen changed the title fix use of rbindlist(..., use.names=FALSE, fill=TRUE) in merge remove use of rbindlist(..., use.names=FALSE, fill=TRUE) in merge Dec 27, 2023
Copy link
Member

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Leaving unmerge in case Jan has anything else first, but feel free to merge & we can handle other issues in follow-up if need be.

@jangorecki jangorecki merged commit 62b1044 into master Dec 27, 2023
5 checks passed
@jangorecki jangorecki deleted the merge_diff_att branch December 27, 2023 16:02
ben-schwen added a commit that referenced this pull request Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rbind in 1.14.3 doesn't like POSIX
3 participants