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

Use proper NA for integer64 on assign #3724

Merged
merged 7 commits into from
Aug 24, 2019
Merged

Use proper NA for integer64 on assign #3724

merged 7 commits into from
Aug 24, 2019

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Jul 23, 2019

Closes #3723
Closes #1459

There's probably a better way to do this, but I couldn't think of one.

The problem is writeNA is integer64-aware, but allocNAVector is not because it operates on SEXPTYPE so it forgets about the source vector & its attributes.

Landed on defining allocNAVectorLike to allocate an empty vector akin to x. Played with two other versions before scrapping:

  • Completely re-define allocNAVector to operate as allocNAVectorLike does. This didn't work because of the usages in fmelt, e.g.. There, allocNAVector isn't matching a specific column, but rather the bumped-to type in reshaping (using the type hierarchy on the measure.vars).
  • Add a parameter is_i64 to allocNAVector and writeNA. First this feels pretty hackish (what other attributes will need a new/different parameter?), and second I got stuck because some usages of writeNA are sending an SEXP and I'm not confident we can always INHERITS(x, char_integer64) in those cases in rbindlist.

@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #3724 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3724      +/-   ##
==========================================
+ Coverage   99.41%   99.41%   +<.01%     
==========================================
  Files          71       71              
  Lines       13210    13216       +6     
==========================================
+ Hits        13133    13139       +6     
  Misses         77       77
Impacted Files Coverage Δ
src/dogroups.c 96.97% <100%> (ø) ⬆️
src/assign.c 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfca73d...a88f373. Read the comment docs.

inst/tests/tests.Rraw Outdated Show resolved Hide resolved
@jangorecki
Copy link
Member

jangorecki commented Aug 13, 2019

There is a more tricky case that is not being handled by this PR, a sub-assign NA to existing int64 column

data.table(a=1:2, b=bit64::as.integer64(1:2))[2L, c("a","b") := NA][]
#       a                   b
#   <int>               <i64>
#1:     1                   1
#2:    NA 9218868437227407266

You might not try to address it in this PR, after having #3765 merged it should be easier.

@mattdowle mattdowle changed the title Closes #3723 and #1459 -- use proper NA for integer64 on assign Use proper NA for integer64 on assign Aug 23, 2019
@mattdowle mattdowle added this to the 1.12.4 milestone Aug 23, 2019
@mattdowle mattdowle merged commit 96be5a9 into master Aug 24, 2019
@mattdowle mattdowle deleted the allocNA_i64 branch August 24, 2019 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants