Add JoinReftype to Relational Joins (to add asof, positional, dependent joins) #7987
Conversation
|
looks good to me! |
tools/rpkg/R/relational.R
Outdated
| } | ||
|
|
||
| rel_join <- function(left, right, conds, join = c("inner", "left", "right", "outer", "cross", "semi", "anti")) { | ||
| rel_join_ <- function(left, right, conds, | ||
| join = c("inner", "left", "right", "outer", "cross", "semi", "anti", "asof"), |
There was a problem hiding this comment.
asof is a JoinRefType, not a JoinType.
| expect_error(rel_join(test_df1, test_df2, cond, ref_type="asof"), "Binder Error") | ||
| }) | ||
|
|
||
| test_that("multiple conditions for asof join throws error", { |
| expect_equal(rel_df, expected_result) | ||
| }) | ||
|
|
||
| test_that("ASOF join works", { |
There was a problem hiding this comment.
Maybe test outer ASOF joins?
tools/rpkg/src/relational.cpp
Outdated
| } else if (join_ref_type == "asof") { | ||
| join_ref = JoinRefType::ASOF; | ||
| } else if (join_ref_type == "dependent") { | ||
| join_ref = JoinRefType::DEPENDENT; |
There was a problem hiding this comment.
Dependent joins are only used internally for subquery flattening and shouldn't be explicitly created by the user.
| }) | ||
|
|
||
| test_that("Invalid asof join condition throws error", { | ||
| dbExecute(con, "CREATE OR REPLACE MACRO neq(a, b) AS a <> b") |
There was a problem hiding this comment.
Can we test positional joins as well?
…ft outer asof joins
This would eventually trigger in OSX workflow
|
Waiting for CI to pass on my own fork before marking ready to review Tmonster#57 |
|
|
||
| // run a positional cross product | ||
| auto join_ref_type = JoinRefType::POSITIONAL; | ||
| vcross = v1->CrossProduct(v2, join_ref_type); |
There was a problem hiding this comment.
This should be a positional join not a positional cross product. Could we move this to the JoinRef as well?
There was a problem hiding this comment.
I'm not sure what you mean, there are join_types and join_ref types. Do you mean just make it a regular join? It would look something like
vcross = v1->Join(v2, "v1.i=v2.j", JoinType::INNER, join_ref_type);
There was a problem hiding this comment.
Something like v1->PositionalJoin(v2) would make more sense to me
There was a problem hiding this comment.
hmm, I feel like adding the type of join to function name might add unnecessary overhead/maintenance. If we do, we may also end up creating relational functions like v1->InnerJoin(v2), v1->LeftJoin(v2) , v1->AsOfJoin(v2) etc. Since the CrossProduct relation already exists I think we should keep someone is already using it. But I think we should stick with
vcross = v1->Join(v2, "v1.i=v2.j", JoinType::INNER, JoinRefType::POSITIONAL) for positional joins
|
Thanks! Looks good |
…relational Add JoinReftype to Relational Joins (to add asof, positional, dependent joins)
…relational Add JoinReftype to Relational Joins (to add asof, positional, dependent joins)
…relational Add JoinReftype to Relational Joins (to add asof, positional, dependent joins)
…nt joins) - Merge pull request duckdb/duckdb#7987 from Tmonster/add_asof_join_to_relational - Merge pull request duckdb/duckdb#8227 from Tmonster/remove_R_warning: Change join ref type to cross if join type is cross and join ref type is regular - Merge pull request duckdb/duckdb#8274 from krlmlr/b-relational-join-tests: Fix handling of cross joins
…nt joins) - Merge pull request duckdb/duckdb#7987 from Tmonster/add_asof_join_to_relational - Merge pull request duckdb/duckdb#8227 from Tmonster/remove_R_warning: Change join ref type to cross if join type is cross and join ref type is regular - Merge pull request duckdb/duckdb#8274 from krlmlr/b-relational-join-tests: Fix handling of cross joins
Currently users can't specific the join ref type for join relations. This means they cannot create join relations that perform
asof,positional, ordependentjoins.In this PR we add the join ref type to the join relation class, while keeping the old constructor for the substrait package so that CI doesn't fail.