by = .EACHI key fix 4603

[ColeMiller1]

Closes #4603.
Closes #4911.

During by = .EACHI, the sorted attribute could be incorrectly assigned. Because the on = .(lhs = rhs), the order of the sorted attributes may not match the order of the by columns.

A second issue is that during normal by = operations, each column is keyed. However, since not all columns in Y of X[Y] are not guaranteed to be keyed, we cannot assume that all the columns are keyed, per existing behavior:

data.table/R/data.table.R

Line 1835 in 263b53e

setattr(ans,"sorted",names(ans)[seq_along(grpcols)])

[codecov]

Codecov Report

Merging #4917 (95e4c95) into master (ec1259a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 95e4c95 differs from pull request most recent head 81e13ec. Consider uploading reports for the commit 81e13ec to get more accurate results

@@           Coverage Diff           @@
##           master    #4917   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files          73       73           
  Lines       14429    14430    +1     
=======================================
+ Hits        14346    14347    +1     
  Misses         83       83           

Impacted Files	Coverage Δ
R/data.table.R	99.94% <100.00%> (+<0.01%)	⬆️

---

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 ec1259a...81e13ec. Read the comment docs.

[mattdowle]

I removed the length(f__)==1L too. That was there, iiuc, from before #3443 when is.sorted() returned false for a single NA. Since is.sorted() is true for all length 1 input, the || length(f__)==1L is superfluous now.
Also moved the roll=FALSE first before && is.sorted(f__) to save the call to is.sorted() when roll==TRUE.

[MichaelChirico]

chmatch not match too. i can fix if you're AFK now.

[mattdowle]

good spot. done.

[mattdowle]

btw, I thought we had a isLeadingSubsetOfKey() helper function, or similar name, as we do that op quite a bit. But when I did a few greps I couldn't find anything.

[MichaelChirico]

I was exactly thinking that we should just have a helper for this by now...

[MichaelChirico]

is there a reason for identical() over all( . == . ) here? identical is a heavier test including things like attributes right?

also at a glance I would do head() before match()

[mattdowle]

Good point. == is an alloc returning logical vector which all() then passes over. It's the alloc that's nice to avoid; usually fast but could trigger a gc. identical is a binary compare so, when there aren't any attributes to check, it's faster than all(.==.), iirc some tests from years back posted somewhere where identical was fastest. But could be wrong and would be surprised if there's much in it in this small case. Then there's call overhead of .Internal vs .Primitive vs S3-method of the various options (identical vs all vs all.equal).

Yep head before match good idea. Will do.

[mattdowle]

caveat ... == for a length 1 vector is a special case that doesn't allocate, just for the reason that R has global internal constants for TRUE, FALSE, and NA_LOGICAL. So the result of x==1L when x is length 1L will be one of those global pointers without an alloc. As always, IIUC.