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

setkey works on tables with complex columns #3688

Merged
merged 8 commits into from
Jul 18, 2019
Merged

setkey works on tables with complex columns #3688

merged 8 commits into from
Jul 18, 2019

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Jul 10, 2019

Closes #1444
Closes #3704

@codecov
Copy link

codecov bot commented Jul 10, 2019

Codecov Report

Merging #3688 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3688      +/-   ##
=========================================
+ Coverage   98.29%   98.3%   +0.01%     
=========================================
  Files          69      69              
  Lines       13285   13267      -18     
=========================================
- Hits        13058   13042      -16     
+ Misses        227     225       -2
Impacted Files Coverage Δ
src/init.c 100% <ø> (ø) ⬆️
R/setkey.R 98.69% <ø> (-0.01%) ⬇️
src/reorder.c 100% <100%> (+2.53%) ⬆️

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 cc0a218...14195c3. Read the comment docs.

@MichaelChirico
Copy link
Member Author

MichaelChirico commented Jul 10, 2019

raw columns continue to fail:

DT = data.table(ID = 2:1, r = as.raw(0:1))
setkey(DT, ID)

Error in setkeyv(x, cols, verbose = verbose, physical = physical) :
Item 2 of list is type 'raw' which isn't yet supported (SIZEOF=1)

Should we try and add that to this PR as well?

Ditto expression:

DT = data.table(ID = 2:1, x = expression(1, 2))
setkey(DT, ID)

Error in setkeyv(x, cols, verbose = verbose, physical = physical) :
Item 2 of list is type 'expression' which isn't yet supported (SIZEOF=0)

(not sure how SIZEOF can be 0 🤔

If not, then we can at least use these for codecov of the 4/8/16 error

@MichaelChirico
Copy link
Member Author

Trying to do coverage and came across this function: setrev

I don't see it used anywhere and it's not exported... shall we delete it? Otherwise we should file an issue to have it exported.

grep to show it's unused:

grep -r "setrev" . | grep -v "Rproj" | grep -v "tests/tests" | grep -v "Binary"
./R/setkey.R:setrev = function(x) .Call(Csetrev, x)
./src/init.c:SEXP setrev();
./src/init.c:{"Csetrev", (DL_FUNC) &setrev, -1},
./src/reorder.c:SEXP setrev(SEXP x) {

@MichaelChirico
Copy link
Member Author

Have merged what's done here into #3701 because the reorder work was needed to get setkey working there. Closing this & filing the above comment as an issue.

@MichaelChirico
Copy link
Member Author

reopening to help reduce the size&complexity of #3701

@mattdowle mattdowle added this to the 1.12.4 milestone Jul 18, 2019
@mattdowle mattdowle changed the title Closes #1444 -- setkey works on tables with complex columns setkey works on tables with complex columns Jul 18, 2019
@Rdatatable Rdatatable deleted a comment from codecov bot Jul 18, 2019
@mattdowle mattdowle merged commit f837d50 into master Jul 18, 2019
@mattdowle mattdowle deleted the cplx_setkey branch July 18, 2019 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove setrev function Support for grouping/ordering/computing on columns of type complex
2 participants