Repository navigation
Error messages from merge() somewhat confusingly "swap" 'x' and 'i' prefixes #6641
Description
Activity
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
Reacted by Michael Chirico and r2evansGreat 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.
Reacted by Ricardo VillalbaHi @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 messagestopf("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 tostopf("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.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 runsDT2[DT1, on='a'], i.e., in the reverse order of the arguments tomerge(). And moreover,DT2[DT1, on='a']usesxprefix to refer to the "outer" table,iprefix for the inner table (matching the names of the arguments to[), whereas themerge()arguments are namedxandy, respectively.The goal of this issue is to match the output to users' expectations in both cases (joins with
x[i]andmerge(x, y)).It will probably require some understanding of condition objects in R:
Reacted by Abhishek FarshwalHi @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.
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.
Reacted by MukulUnderstood @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?
Yes, and mention that it is advised to use
y[x]instead ofmerge(x, y)Reacted by Mukul and Toby Dylan HockingI'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 thatiis, in fact, adata.table. In fact that don't even need to care, strictly, thatxis adata.table.merge()signals the code's intent clearly and locally.Thanks, @MichaelChirico, it's a helpful clarification on the value of merge().
The solution in which
bmerge.Rsignals a custom error object with details, andmerge()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 formerge.Rto use. - In
merge.R: We will usetryCatcharound 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
bmergeerrors if seen by amerge()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?
- In
I like the sound of that, thank you
Reacted by Mukul
Note that the
x.prefix applies toDT2, while thei.prefix applies toDT1, while the user provided them in order(DT1, DT2).At root is that under the hood,
merge(x,y, ...)is constructed as a join withy[x, ...].It would be most consistent with users' expectations of
merge()if the prefixes wereDT1 : x.,DT2: y., or perhaps even better if the errors matchedsuffixes=. A really subtle (and back-incompatible) way to thread the needle here would be to change themerge.data.table()defaults to besuffixes=c(".i", ".x").A different approach (and probably the most practical at this point) is just to emphasize this consideration in the docs.