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

rbindlist segfault for fill=TRUE and usenames=FALSE #5468

Merged
merged 4 commits into from
Dec 8, 2023

Conversation

ben-schwen
Copy link
Member

@ben-schwen ben-schwen commented Sep 16, 2022

Closes #5444

  • fix
  • tests
  • update news

@jangorecki
Copy link
Member

News is not needed as this was in dev only

@ben-schwen ben-schwen added the dev label Sep 16, 2022
@ben-schwen
Copy link
Member Author

I changed the behavior of test 2003.5 introduced in #5263 since for me it seems more natural if the data.table gains the column name of the table with the most columns and not a new one.

This is no breaking change since fill=TRUE with usenames=FALSE is only available in current dev version 1.14.3

@codecov
Copy link

codecov bot commented Sep 17, 2022

Codecov Report

Merging #5468 (c7c5ce2) into master (2f67531) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5468   +/-   ##
=======================================
  Coverage   99.51%   99.51%           
=======================================
  Files          78       80    +2     
  Lines       14756    14772   +16     
=======================================
+ Hits        14684    14700   +16     
  Misses         72       72           
Impacted Files Coverage Δ
src/fread.c 99.41% <ø> (ø)
src/init.c 100.00% <ø> (ø)
R/notin.R 100.00% <100.00%> (ø)
src/negate.c 100.00% <100.00%> (ø)
src/rbindlist.c 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jangorecki
Copy link
Member

I think the names of columns in such case shoybe based on the order of occurrence. Unit test for that is pretty simple, doesn't cover for example case of 3 tables. But the results of the current test looks good (names are based on the order of occurrence of columns), better than before.

@MichaelChirico
Copy link
Member

I changed the behavior of test 2003.5 introduced in #5263 since for me it seems more natural if the data.table gains the column name of the table with the most columns and not a new one.

This is no breaking change since fill=TRUE with usenames=FALSE is only available in current dev version 1.14.3

now that there's new behavior, I think that does warrant a NEWS edit (either adding to the existing item or getting its own)

@jangorecki jangorecki added this to the 1.15.0 milestone Nov 6, 2023
@jangorecki jangorecki merged commit 1b130ef into master Dec 8, 2023
@jangorecki jangorecki deleted the rbindlist_segfault branch December 8, 2023 15:11
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.

rbindlist() crashing R session
3 participants