[cbindlist/mergelist] Add ERR to the mult enum in bmerge.c

[MichaelChirico]

Towards #4370. Support for this value of mult is required by mergepair() later; not actually exposed to use here, so ignoring codecov report. To be tested later.

[MichaelChirico]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• [cbindlist/mergelist] Add some C utilities: NROW/NCOL/isDataTable/isDataList/perhapsDataTable #6433 : 2 dependent PRs (#6434 , #6435 )
• [cbindlist/mergelist] Add ERR to the mult enum in bmerge.c #6432 👈
• [cbindlist/mergelist] Whitespace changes #6431
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @MichaelChirico and the rest of your teammates on Graphite

[github-actions]

Generated via commit 7342791

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	3 minutes and 26 seconds
Installing different package versions	7 minutes and 2 seconds
Running and plotting the test cases	2 minutes and 5 seconds

[MichaelChirico]

@jangorecki is there any user-facing change in this PR? Should it get its own NEWS item/tests directly of [ behavior?

[jangorecki]

Yes new.supported value for mult sounds like user facing change

[MichaelChirico]

OK thanks. Please add NEWS, I think it should get direct tests in tests.Rraw besides what's done implicitly in mergelist.Rraw, too.

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.61%. Comparing base (01233bf) to head (7342791).

Files with missing lines	Patch %	Lines
src/bmerge.c	50.00%	2 Missing ⚠️

Additional details and impacted files

@@                    Coverage Diff                    @@
##           cbind-merge-list-prep    #6432      +/-   ##
=========================================================
- Coverage                  98.62%   98.61%   -0.02%     
=========================================================
  Files                         79       79              
  Lines                      14472    14474       +2     
=========================================================
  Hits                       14273    14273              
- Misses                       199      201       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MichaelChirico]

@jangorecki I'm confused, how is this actually supposed to work?

DT1=data.table(a=1, b=2)
DT2=data.table(a=1, c=3:4)
DT1[DT2, on=.(a >= a), mult='error']
# Error in bmerge(i, x, leftcols, rightcols, roll, rollends, nomatch, mult,  : 
#   Internal error in nestedid: invalid value for 'mult'. Please report to the data.table issues tracker.

That's from:

data.table/R/bmerge.R

Line 166 in 7419796

nqgrp = .Call(Cnestedid, x, xcols[non_equi:length(xcols)], xo, xg, resetlen, mult)

So it looks like we don't actually have support for mult="error" inside [.data.table in this PR chain?

I used on=.(a>=a) to induce a non-equi join based on how bmerge.c is actually changed here for mult=ERR:

data.table/src/bmerge.c

Lines 405 to 440 in 7419796

	// non-equi join
	for (int j=ilow+1; j<iupp; j++) {
	const int k = o ? o[j]-1 : j;
	if (retFirst[k] != nomatch) {
	if (mult == ALL) {
	// for this irow, we've matches on more than one group
	allGrp1[0] = FALSE;
	retFirst[ctr+ilen] = xlow+2;
	retLength[ctr+ilen] = len;
	retIndex[ctr+ilen] = k+1;
	++ctr;
	if (ctr+ilen >= anslen) {
	anslen = 1.1*anslen;
	retFirst = R_Realloc(retFirst, anslen, int); // if fails, it fails inside R_Realloc with R error
	retLength = R_Realloc(retLength, anslen, int);
	retIndex = R_Realloc(retIndex, anslen, int);
	}
	} else if (mult == FIRST) {
	retFirst[k] = (XIND(retFirst[k]-1) > XIND(xlow+1)) ? xlow+2 : retFirst[k];
	retLength[k] = 1;
	} else {
	retFirst[k] = (XIND(retFirst[k]-1) < XIND(xupp-1)) ? xupp : retFirst[k];
	retLength[k] = 1;
	}
	} else {
	// none of the groups so far have filled in for this index. So use it!
	if (mult == ALL) {
	retFirst[k] = xlow+2;
	retLength[k] = len;
	retIndex[k] = k+1;
	// no need to increment ctr of course
	} else {
	retFirst[k] = (mult == FIRST) ? xlow+2 : xupp;
	retLength[k] = 1;
	}
	}

Do we actually need to expose mult="error" through [? It looks like mergelist() only needs bmerge() to support it, not [.

[jangorecki]

Firstly I would check what is the output of the original branch so rule out issues related to conflicts resolution. But afair this PR was not meant to change [ yet, only provide new functions.

[MichaelChirico]

Yea, I get the same error at the end of the chain (#4370). So I think we should not make the change to data.table.R here.

[MichaelChirico]

OK, this PR is G2G.