FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Keep joins grouped by foreign columns from crashing by tlapak · Pull Request #4944 · Rdatatable/data.table · GitHub

/ data.table Public

Keep joins grouped by foreign columns from crashing - #4944

Merged
mattdowle merged 5 commits into
masterfrom
fix4892
May 6, 2021
ConversationCommits (5)ChecksFiles changed
Merged

Keep joins grouped by foreign columns from crashing#4944
mattdowle merged 5 commits into
masterfrom
fix4892

Conversation

tlapak commented Mar 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #4892

So this fixes the reported segfault, but I'm not sure if there isn't something else that's going wrong. Slightly modifying @ColeMiller1's example, we get

library('data.table')
x = data.table(id = 1:4, grp = 1:4, key = 'id')
y = data.table(id = 2:5, key = 'id')
x[y, by = x$grp, .SD, verbose = TRUE]
>        x    id
>    <int> <int>
> 1:     1     2
> 2:     2     3
> 3:     3     4
> 4:     4    NA

x[y, by = grp, .SD, verbose = TRUE]
>      grp    id
>    <int> <int>
> 1:     2     2
> 2:     3     3
> 3:     4     4
> 4:    NA    NA

Is that the correct behaviour? They can't both be correct, right? I'm not really sure what the intended purpose of grouping by a non-member column is. Maybe it would be better to catch this further up.

Also just as a remark, it's not necessary for this to be keyed join to trigger the sagfault.

tlapak added the WIP label Mar 26, 2021

codecov Bot commented Mar 26, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #4944 (4600d3e) into master (900711c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4944   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files          73       73           
  Lines       14459    14461    +2     
=======================================
+ Hits        14376    14378    +2     
  Misses         83       83
Impacted Files Coverage Δ
src/dogroups.c 99.67% <100.00%> (+<0.01%) ⬆️
src/subset.c 99.50% <100.00%> (ø)

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 900711c...4600d3e. Read the comment docs.

This comment has been minimized.

Sign in to view

tlapak commented Mar 30, 2021

Copy link
Copy Markdown
Contributor Author

I thought about it some more and I guess aggregating a join makes sense? But I can't find documentation on it and the behaviour is somewhat unexpected to me. I played around with it a bit more and it works out as shorthand for slightly different join + group by operations:

library('data.table')

x = data.table(id = 1:4, grp = c(2L, 1L, 2L, 1L), key = 'id')
y = data.table(id = 2:5, grp = c(2L, 1L, 2L, 1L), key = 'id')

all.equal(x[y, by=grp, .SD],
          x[y, .(grp=x.grp, id=x.id)][, .SD, grp])
> [1] TRUE

all.equal(x[y, by=.(grp=x$grp), .SD],
          x[y, .(grp=i.grp, id=x.id)][, .SD, grp])
> [1] TRUE

all.equal(x[y, .SD],
          x[y, .(id=i.id, grp=x.grp)])
> [1] TRUE

Although I haven't tried it for more complex examples.
Interestingly, the columns of the i argument aren't available when by is passed a column.

I'm not sure if it's just Windows, but segfaults are difficult to debug for me - shutting down R each time is disruptive.

Good old print debugging... The tooling on Windows can be a bit of a pain.

tlapak removed the WIP label Mar 31, 2021

ColeMiller1 commented Apr 2, 2021

Copy link
Copy Markdown
Contributor

Doh. Sorry for the mistake - I remembered that by = i.col but should have tested by = x.col before commenting.

I think as long as data.table allows for column use from the parent.frame, there will just be some edge cases. The only other thought I have is to probe the by expression to try to determine if the variable is in the data.table. If the variable is in the parent.frame and !is.null(irows) then a warning might be good as it's unclear what the expected output should be. That is, would by = parent_frame_col be subsetted as well or would it be used as is.

The last line of this is what explains the discrepancy. While x has been subsetted, the variable in the parent.frame has not been.

data.table/R/data.table.R

Lines 790 to 800 in ec1259a

if (length(allbyvars)) { ############### TO DO TO DO TO DO ###############
if (verbose) cat("i clause present and columns used in by detected, only these subset:",paste(allbyvars,collapse=","),"\n")
xss = x[irows,allbyvars,with=FALSE,nomatch=nomatch,mult=mult,roll=roll,rollends=rollends]
} else {
if (verbose) cat("i clause present but columns used in by not detected. Having to subset all columns before evaluating 'by': '",deparse(by),"'\n",sep="")
xss = x[irows,nomatch=nomatch,mult=mult,roll=roll,rollends=rollends]
}
if (bysub %iscall% ':' && length(bysub)==3L) {
byval = eval(bysub, setattr(as.list(seq_along(xss)), 'names', names(xss)), parent.frame())
byval = as.list(xss)[byval]
} else byval = eval(bysub, xss, parent.frame())

mattdowle added this to the 1.14.1 milestone May 6, 2021

mattdowle commented May 6, 2021

Copy link
Copy Markdown
Member

Great fix! I confirmed the new test crashed for me too before this fix.

mattdowle merged commit 38aa0b1 into master May 6, 2021
mattdowle deleted the fix4892 branch May 6, 2021 08:42
jangorecki modified the milestones: 1.14.9, 1.15.0 Oct 29, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.15.0

Development

Successfully merging this pull request may close these issues.

Segfault in [.data.table

4 participants

Back | FazBrowse Home | New Git URL