Error messages from merge() somewhat confusingly "swap" 'x' and 'i' prefixes

[MichaelChirico]

DT1=data.table(a=factor('a'))
DT2=data.table(a=1L)
merge(DT1, DT2, by='a')
# Error in bmerge(i, x, leftcols, rightcols, roll, rollends, nomatch, mult,  : 
#   Incompatible join types: x.a (integer) and i.a (factor). Factor columns must join to factor or character columns.

Note that the x. prefix applies to DT2, while the i. prefix applies to DT1, while the user provided them in order (DT1, DT2).

At root is that under the hood, merge(x,y, ...) is constructed as a join with y[x, ...].

It would be most consistent with users' expectations of merge() if the prefixes were DT1 : x., DT2: y., or perhaps even better if the errors matched suffixes=. A really subtle (and back-incompatible) way to thread the needle here would be to change the merge.data.table() defaults to be suffixes=c(".i", ".x").

A different approach (and probably the most practical at this point) is just to emphasize this consideration in the docs.

[rikivillalba]

Perhaps one can raise an errorCondition with a custom 'call' argument such that 'bmerge(i, x, leftcols,...' becomes 'bmerge(i = DT2, x = DT1, leftcols,...' though not standard

[MichaelChirico]

Great idea! I think we can populate custom attributes of the error object raised on the custom class and use those more reliably & transparently than messing with the call.

[Abhishek2634]

Hi @MichaelChirico, I would like to work on this issue. can you assign me this issue, As i have figure out what to do.
in file bmerge.R, line no. 81 is the error message stopf("Incompatible join types: %s (%s) and %s (%s). Factor columns must join to factor or character columns.", xname, x_merge_type, iname, i_merge_type). i have change this to stopf("Incompatible join types: %s (%s) and %s (%s). Factor columns must join to factor or character columns.", iname, i_merge_type, xname, x_merge_type) and its working fine with output error stating factor if D1 is factor type, and D2 as integer type resp.

for reference,
DT1=data.table(a=factor('a'))
DT2=data.table(a=1L)
merge(DT1, DT2, by='a')

its error output will be: Incompatible join types: i.a (factor) and x.a (integer). Factor columns must join to factor or character columns.

[MichaelChirico]

Thanks @Abhishek2634, the issue with your proposed fix is it's incorrect for the "primary" merge interface, namely DT1[DT2, on='a'].

The root issue here is that merge(DT1, DT2, by='a') eventually runs DT2[DT1, on='a'], i.e., in the reverse order of the arguments to merge(). And moreover, DT2[DT1, on='a'] uses x prefix to refer to the "outer" table, i prefix for the inner table (matching the names of the arguments to [), whereas the merge() arguments are named x and y, respectively.

The goal of this issue is to match the output to users' expectations in both cases (joins with x[i] and merge(x, y)).

It will probably require some understanding of condition objects in R:

https://adv-r.hadley.nz/conditions.html

[Mukulyadav2004]

Hi @MichaelChirico @tdhock
As for merge(x,y), the meaning of x and y is different from bmerge's internal x and i.
Thus to ensure clarity for merge without altering the correct messages for DT1[DT2] users, we will modify merge.R so that it catches the detailed error from bmerge.R and rephrases it to match the merge(x,y) user's perspective.

Thus in this modification our steps would be-
1.) In bmerge.R, when such an incompatibility is found, it signals a custom error object. This object carry the column names and types as attributes, while its default message string remains same.
2.) In merge.R, this specific error object is caught. The handler uses these attributes to show a new message, correctly mapping to merge(x,y) arguments.

What do you think about this? If this right to you may I open a PR for this.

[jangorecki]

I would just go with docs update. Why anyone would want to use merge()? Let's not add code complexity for something that should be avoided.

[Mukulyadav2004]

Understood @jangorecki
I will draft a section for the merge.Rd page to:
Clarify that internal errors might use x. and i. prefixes based on the y[x] join structure and Also explaining how these map to the x and y arguments of merge().

Does this seem correct to you?

[jangorecki]

Yes, and mention that it is advised to use y[x] instead of merge(x, y)

[MichaelChirico]

I'm not sure that's ideal, I often use merge() for code clarity. Code reader does not need to read maybe dozens or hundreds of lines over maybe multiple files to figure out that i is, in fact, a data.table. In fact that don't even need to care, strictly, that x is a data.table. merge() signals the code's intent clearly and locally.

[Mukulyadav2004]

Thanks, @MichaelChirico, it's a helpful clarification on the value of merge().

The solution in which bmerge.R signals a custom error object with details, and merge() catches it to rephrase the message for its x and y arguments is minimal :

• In bmerge.R: Only the specific stopf() for this factor incompatibility is changed to stop(custom_condition). The default message for joins remains same, and it just adds attributes for merge.R to use.
• In merge.R: We will use tryCatch around the join call, with a handler uses these attributes to show a new message.

This directly fixes the x/i confusion for merge() users, making its error output -
Error: Incompatible join types: x.a (type of x) and i.a (type of y))

And then after we'll document in merge.Rd-

• Explaining how any remaining low-level bmerge errors if seen by a merge() user - map their x./i. prefixes to merge's arguments.
• And, as suggested by @jangorecki, to use y[x] instead of merge(x, y).

Does this approach, targeting clear errors for merge() itself, seems correct to you or going with only docs update is better option?

[MichaelChirico]

I like the sound of that, thank you