-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add DataFrame support for INTERSECT
and update readme
#1258
Conversation
Make DataFrame users have the |
@@ -231,6 +231,29 @@ impl DataFrame for DataFrameImpl { | |||
.build()?, | |||
))) | |||
} | |||
|
|||
fn intersect(&self, dataframe: Arc<dyn DataFrame>) -> Result<Arc<dyn DataFrame>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like there is some code duplication between this and the one in sql planner, perhaps better to move this into logical plan builder so it can be reused in both places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be valuable
datafusion/src/dataframe.rs
Outdated
@@ -375,4 +375,19 @@ pub trait DataFrame: Send + Sync { | |||
/// # } | |||
/// ``` | |||
fn registry(&self) -> Arc<dyn FunctionRegistry>; | |||
|
|||
/// Calculate the intersect two [`DataFrame`]s. The two [`DataFrame`]s must have exactly the same schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Calculate the intersect two [`DataFrame`]s. The two [`DataFrame`]s must have exactly the same schema | |
/// Calculate the intersection of two [`DataFrame`]s. The two [`DataFrame`]s must have exactly the same schema |
&LogicalPlanBuilder::from(left_plan) | ||
.join_detailed(&right_plan, JoinType::Semi, join_keys, true)? | ||
.build()?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think once we merge #1259 this could just call LogicalPlanBuilder::intersect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cae6e55
to
d045dbd
Compare
The ticket has been done! |
INTERSECT
and update readme
Looks great -- thank you @xudong963 |
Which issue does this PR close?
Closes #1082
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?