The decreased code coverage is due to a section, where I implement the behaviour for columns where all of the following return FALSE: is.logical, is.integer, base::is.double, is.character, is.factor., is.integer64
I haven't managed to come up with an example that satisfies all of these. I tried raw or complex vectors, but these fail earlier in forder and can't be used to cover this section. There are three options from my perspective:

1. Find a test column that satisfies the above-mentioned criteria.
2. Remove the code that deals with this case. If in reality the case occurs, it will raise an error and can be fixed. If we can't come up with an example for the tests, this is very unlikely to occur.
3. Live with the decreased coverage, leave the code in and don't test it.

If option 1 turns out to be infeasible, I would opt for 2.

Looks great and much more robust. New tests are extensive!

1. How about defining a data matrix in the code, something like this :

i type down rows
x type along cols
                1     2     3     4   ...
1. logical      y     w     w     w
2. integer      e     y     w
3. intAsReal    e     w     w
4. reallyReal   e    cw           y
5. ...

where y = yes types match as expected normally, e = error, w = no coerce but warn, cw = coerce and warn
Then the code would look up the behaviour from the matrix and do that action code. That would be easier to read, debug and maintain; e.g. adding new types in future. For example, in writing this comment I looked at position [3,3] and changed it from "y" to "w". Since, although the types match in that case we'd want to suggest to the user they change both sides from double to integer for space and time efficiency.

2. For the "other" type, maybe that should just be error? What happens now if either or both typeof()=="complex" for example? I'd expect/hope error. That error could be tested and then the coverage would be 100%.

3. Needs a new item in NEWS.md bug fix section please.

Great, I like your suggested behaviour very much!

• Concerning 1: I will update so that the matrix is used for lookup. For the coerce case, we need to specify if i or x gets coerced, so I will introduce ci (coerce i so that it matches x) and cx (coerce x so that it matches i).
• Concerning 2: I will implement that any column that !is.integer & !base::is.double& !is.character & !is.factor & !inherits(col,"integer64") throws an error. Currently, complex column in x throws an error before arriving at bmerge in forder when attempting to create an index. Maybe, complex in i will work to cover the code in unit tests, I will test that.
• Concerning 3: I will update the NEWS.md

I have refactored the code according to @mattdowle 's proposals with 2 exceptions:

1. I had difficulties defining the warning message for the flagw for 'no coercion but warning': what should it warn about? Would it only be for intAsReal columns as suggested by @mattdowle? Then it could only be entered at very specific positions in the matrix. This is not good from my perspective. Therefore, I would propose to treat this warning seperately, outside the matrix. After further thought, I decided not to implement it at all for a very simple reason: I don't believe that bmerge is the correct place to warn users about storing integers as integers. If that is intended, it should happen on creation of intAsReal columns in my opinion. I am very open for other suggestions on this topic. The current possible coercion strategies are the following:

  ##------------------|-----------------------------------------------------
  ## y (yes):         | no coercion necessary, types match
  ##------------------|-----------------------------------------------------
  ## e (error):       | throw an error because of incompatible types
  ##------------------|-----------------------------------------------------
  ## ci (coercion i): | coerce the column in i to the type of the column in x
  ##------------------|-----------------------------------------------------
  ## ciw:             | same as above, but with warning.
  ##------------------|-----------------------------------------------------
  ## cx (coercion x): | coerce the column in x to the type of the column in i
  ##------------------|-----------------------------------------------------
  ## cxw:             | same as above, but with warning.
  ##------------------|-----------------------------------------------------

2. Originally, we wanted to issue warnings to users on joins with non-matching types (currently, there are only verbose message warnings). I did not do that so far since this would affect > 100 existing unit tests, including cases, where internal data.table code would cause warnings. So, before doing so, I would propose to clean the code and tests for unnecessary type mismatches and then implementing the warnings, which is as simple as replacing ci and cx with ciw and cxw in the type conversion matrix respectively (see table under 1.).

Should the type conversion matrix be symmetrical* (assuming ci <-> cx exchange upon transpose)? That is, do we expect that for any two types A, B the following should hold:

  [A, B] = y    <=>  [B, A] = y
  [A, B] = e    <=>  [B, A] = e
  [A, B] = ci   <=>  [B, A] = cx
  [A, B] = ciw  <=>  [B, A] = cxw

Currently it doesn't, for example there's ct["integer", "logical"] = "e" whereas ct["logical", "integer"] = "ci" (there are other mismatched pairs like this too).

You are right, @st-pasha, I also think that this would be the ideal behavior. However, this would alter existing behavior beyond fixing bugs IMHO. That is why I didn't implement it so far. If there is an agreement to change the behavior to be symmetrical as suggested by you, I am more than willing to implement. Note, however, that very likely existing tests have to be updated.

My suggestion is to add warnings and make matrix symmetric in this PR but as option. This is big breaking change and it might need to wait for more than 1 release cycle to be changed to TRUE by default. We need to release soon to CRAN, so putting this as an option gives us much more time to check revdeps and inform maintainers. This we can do already after release to CRAN so maintainers can smoothly migrate by using option on recent CRAN build. Later before future releases we can check how many packages adapted to new rules and depending on that change default at some point. @mattdowle what do you think?

While I like the idea of using this PR to consolidate type conversion behaviour in joins in general, we should remember that its primary goal is to fix two pretty terrible bugs in master:

library(data.table)
dt <- data.table(intCol = 1:2, numCol = c(1.5, 2.5), factCol = factor(c("a","a")))
dt[dt, on="intCol==numCol", nomatch = 0L]
#   intCol numCol factCol i.intCol i.factCol
#  1:      1         1.5          a           1           a
#  2:      2         2.5          a           2           a
## 1.5 is joine dto 1 and 2.5 to 2 !

dt[dt, on="intCol==factCol", nomatch = 0L]
#   intCol numCol factCol i.intCol i.numCol
# 1:       a       1.5       a          1         1.5
# 2:       a       1.5       a          2         2.5
## 1 gets joined to "a" since "a" is factor level 1.

So maybe, if the decision on further changes needs more discussion, it can be seperated and this PR merged as a bugfix.

@mattd As you are going to look at bmerge, could you also briefly evaluate how difficult it would be to split current bmerge into bmergeR and bmerge C for future releases?

@jangorecki I am not sure, what you are hinting at with bmergeR and bmergeC. Currently, the R function bmerge is used to take care about type conversions and factor level consolidations. Then, Cbmerge is called to do the real merge.

@MarkusBonsch I mean separation of code as in fread and fwrite, where funR is used for R C API, and fun is used to operate in plain C. This allows to use those inner function without going through R C API. AFAIR currently both depends on R C API.

Thank you so much for taking care of trimws @mattdowle. I didn't find the time. If anything else pops up, I will try to fix it.

ciw and cxw aren't used anywhere in the ct matrix so there isn't any coverage on the warnings. Maybe the warning code can be dropped then, or if not, there should be entries using them. Still looking ...