Add support for CROSS JOIN through full_join over non-overlapping columns
#24
Conversation
R/tbl-lazy.R
Outdated
| @@ -201,7 +201,14 @@ distinct_.tbl_lazy <- function(.data, ..., .dots = list(), .keep_all = FALSE) { | |||
| add_op_join <- function(x, y, type, by = NULL, copy = FALSE, | |||
| suffix = c(".x", ".y"), | |||
| auto_index = FALSE, ...) { | |||
| by <- common_by(by, x, y) | |||
| by_intersect <- intersect(tbl_vars(x), tbl_vars(y)) | |||
There was a problem hiding this comment.
This doesn't handle the case of a full join where by is a named vector that maps the names of the join columns in the left table to the join columns in the right table, as in:
full_join(tbl_left, tbl_right, by = c("xl" = "xr"))There was a problem hiding this comment.
Thanks for the reference, no change here.
|
Let's please discuss at tidyverse/dplyr#2924 whether this is the best approach for implementing cross joins. |
|
I think it's ok to merge this as a short term work around for the absence of |
R/sql-generic.R
Outdated
| stop("Unknown join type:", type, call. = FALSE) | ||
| ) | ||
|
|
||
| select <- sql_join_vars(con, vars) | ||
|
|
||
| on <- sql_vector( | ||
| on <- if (length(by$x) + length(by$y) <= 0) NULL else sql_vector( |
There was a problem hiding this comment.
Can you please pull this out into:
if (...) {
on <- ...
} else {
on <- ...
}Or maybe even consider pulling out into own function?
R/tbl-lazy.R
Outdated
| @@ -201,7 +201,14 @@ distinct_.tbl_lazy <- function(.data, ..., .dots = list(), .keep_all = FALSE) { | |||
| add_op_join <- function(x, y, type, by = NULL, copy = FALSE, | |||
| suffix = c(".x", ".y"), | |||
| auto_index = FALSE, ...) { | |||
| by <- common_by(by, x, y) | |||
| by_intersect <- intersect(tbl_vars(x), tbl_vars(y)) | |||
| by <- if (length(by_intersect) > 0 || !identical(type, "full") || !is.null(by)) { | |||
There was a problem hiding this comment.
Similarly here - I really don't like this style of if-else because it's easy to miss that the entire block gets assigned to some variable.
tests/testthat/test-joins.R
Outdated
| @@ -92,3 +92,8 @@ test_that("sql generated correctly for all sources", { | |||
|
|
|||
| expect_equal_tbls(xy) | |||
| }) | |||
|
|
|||
| test_that("full join is promoted to cross join for no overlapping variables", { | |||
| result <- df1 %>% full_join(df2) %>% collect() | |||
There was a problem hiding this comment.
I don't like making this the default because if you accidentally supply the wrong data frames you might create a query that takes hours to run. Could we make it explicit, i.e. by = character()?
|
Thanks! |
Fix tidyverse/dplyr#2924 by adding support for cross joins.
Currently,
Triggers:
Error:byrequired, because the data sources have no common variables.This PR implements
CROSS JOINSsupport forfull_joinover non-overlapping columns.