Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inline select() in *_join() #875

Merged
merged 16 commits into from
Aug 1, 2022
Merged

Inline select() in *_join() #875

merged 16 commits into from
Aug 1, 2022

Conversation

mgirlich
Copy link
Collaborator

@mgirlich mgirlich commented May 18, 2022

Closes #867.

library(dplyr, warn.conflicts = FALSE)
library(dbplyr, warn.conflicts = FALSE)

lf <- lazy_frame(x1 = 10, a = 1, y = 3, .name = "lf1")
lf2 <- lazy_frame(x2 = 10, b = 2, z = 4, .name = "lf2")

left/right/full/inner_join()

left_join(
  lf %>% select(a2 = a, x = x1),
  lf2 %>% select(x = x2, b),
  by = "x"
)

Before

SELECT `a2`, `LHS`.`x` AS `x`, `b`
FROM (
  SELECT `a` AS `a2`, `x1` AS `x`
  FROM `lf1`
) `LHS`
LEFT JOIN (
  SELECT `x2` AS `x`, `b`
  FROM `lf2`
) `RHS`
  ON (`LHS`.`x` = `RHS`.`x`)

After

SELECT `a` AS `a2`, `x1` AS `x`, `b`
FROM `lf1` AS `LHS`
LEFT JOIN `lf2` AS `RHS`
  ON (`LHS`.`x1` = `RHS`.`x2`)

semi/anti_join()

semi_join(
  lf %>% select(a2 = a, x = x1),
  lf2 %>% select(x = x2, b),
  by = "x"
)

Before

SELECT * FROM (
  SELECT `a` AS `a2`, `x1` AS `x`
  FROM `lf1`
) `LHS`
WHERE EXISTS (
  SELECT 1 FROM (
  SELECT `x2` AS `x`, `b`
  FROM `lf2`
) `RHS`
  WHERE (`LHS`.`x` = `RHS`.`x`)
)

After

SELECT `a` AS `a2`, `x1` AS `x`
FROM `lf1` AS `LHS`
WHERE EXISTS (
  SELECT 1 FROM (
  SELECT `x2` AS `x`, `b`
  FROM `lf2`
) `RHS`
  WHERE (`LHS`.`x1` = `RHS`.`x`)
)

The semi/anti_join() could be improved further but I'm not sure it's worth it (mostly interesting when the join columns have different names and one uses rename() instead of using a named by).

@mgirlich mgirlich added this to the 2.3.0 milestone May 18, 2022
* remove default `by = NULL` as it does not work anyway
* remove default `anti = FALSE` because anti comes before required argument `by`
@mgirlich mgirlich mentioned this pull request May 27, 2022
@@ -7,6 +7,9 @@ lazy_join_query <- function(x,
by,
suffix = c(".x", ".y"),
na_matches = c("never", "na"),
group_vars = NULL,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add_group_by() adds the field group_vars to any lazy_query object. Similarly for order_vars and frame. So it makes sense to be able to explicitly set them when creating these objects.

@mgirlich mgirlich requested a review from hadley July 31, 2022 08:42
@mgirlich
Copy link
Collaborator Author

@hadley Would be great if you could give feedback

  • whether you see problems with inlining
  • you see edge cases that need to be tested

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your logic makes sense to me, and I can see any obvious holes in it.

Is it worth including a test for some of the suffix cases? I'm not sure how much that overlaps with this code.

R/lazy-select-query.R Outdated Show resolved Hide resolved
R/lazy-select-query.R Show resolved Hide resolved
@@ -177,7 +177,8 @@
i Do you need to move arrange() later in the pipeline or use window_order() instead?
Output
<SQL>
SELECT * FROM (
SELECT *
FROM (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like the FROM on its own line.

@mgirlich mgirlich merged commit c01b0c9 into main Aug 1, 2022
@mgirlich
Copy link
Collaborator Author

mgirlich commented Aug 1, 2022

Is it worth including a test for some of the suffix cases?

It should not but rather be safe than sorry. So I added tests for suffix cases and named by.

@mgirlich mgirlich deleted the join-after-select branch August 1, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combine select() + *_join() in a single query
2 participants