Joins with custom suffixes

[christophsax]

Support for the suffix argument in joins.

This can be directly used as the suffixes argument in merge.data.table. The two non-merge methods, semi_join and anti_join, do not have a suffix argument.

Also added a small test.

fixes #40.

[lionel-]

Thanks. You can use git pull --rebase to avoid those merge commits by the way.

[krlmlr]

Looks good. I wonder if extracting a function with argument list x, y, by, copy, suffix, all.x, all.y will improve this code.

[krlmlr]

Could you please write

out <- merge(
  x, y, by.x = ...,
  all = FALSE, ..., allow.cartesian = TRUE
)

We do have a style guide, but I'm not sure this particular part is covered.

[lionel-]

I think if the first argument is on a newline, all arguments should be on their own line and the closing parentheses. Otherwise you fill the lines to the 72nd or 80th column and leave the closing parenthese with the final argument.

[krlmlr]

I'd admit multiple related arguments on the same line in multi-line calls. No need to occupy too much vertical space. What's the rationale behind your approach?

[lionel-]

We use either a vertical arrangement or a horizontal one. Here I would use the latter. I don't think the layout you suggested is standard in the tidyverse.

[krlmlr]

This decision will be more important in the styler project, CC @lorenzwalthert. For this PR I'm fine with either way.

[christophsax]

If I stick to this section in the style guide and to @krlmlr's suggestion to put related arguments on one line, I would write:

merge(
  x, y, 
  by.x = by$x, by.y = by$y, 
  all.x = TRUE, 
  suffixes = suffix, 
  allow.cartesian = TRUE
)

Ok?

[lionel-]

Looks good.

[christophsax]

And if we extract it in a function, we would have something like this?

join_using_merge <- function(x, y, by, copy, suffix, 
                             all.x = FALSE, all.y = FALSE){
  by <- dplyr::common_by(by, x, y)
  y <- dplyr::auto_copy(x, y, copy = copy)
  out <- merge(
    x, y, 
    by.x = by$x, by.y = by$y, 
    all.x = all.x, all.y = all.y
    suffixes = suffix, 
    allow.cartesian = TRUE
  )
  grouped_dt(out, groups(x)) 
}

#' @rdname join.tbl_dt
inner_join.data.table <- function(x, y, by = NULL, copy = FALSE, 
                                  suffix = c(".x", ".y"), ...){
  join_using_merge(x, y, by = by, copy = copy, suffix = suffix)
}

#' @rdname join.tbl_dt
left_join.data.table <- function(x, y, by = NULL, copy = FALSE, 
                                  suffix = c(".x", ".y"), ...){
  join_using_merge(x, y, by = by, copy = copy, suffix = suffix, all.x = TRUE)
}

#' @rdname join.tbl_dt
right_join.data.table <- function(x, y, by = NULL, copy = FALSE, 
                                  suffix = c(".x", ".y"), ...){
  join_using_merge(x, y, by = by, copy = copy, suffix = suffix, all.y = TRUE)
}

#' @rdname join.tbl_dt
full_join.data.table <- function(x, y, by = NULL, copy = FALSE, 
                                  suffix = c(".x", ".y"), ...){
  join_using_merge(x, y, 
    by = by, 
    copy = copy, 
    suffix = suffix, 
    all.x = TRUE, all.y = TRUE
    )
}

[christophsax]

@krlmlr, happy with the code above? Would commit in that case.

[krlmlr]

I like the join_using_merge() function. Please unindent the closing paren in the call in full_join.data.table().

[krlmlr]

Thanks, just a few very minor remarks before merge.

[krlmlr]

Please review indention.

[krlmlr]

Same.

[krlmlr]

Could you please make this a bit more verbose, and credit yourself?

[krlmlr]

Thanks!