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.

Sign up for GitHub

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

Jump to bottom

MINOR: Implement serde for join filter #2649

Merged
merged 1 commit into from
May 30, 2022
Merged

MINOR: Implement serde for join filter #2649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions datafusion/proto/proto/datafusion.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ message JoinNode {
repeated datafusion.Column left_join_column = 5;
repeated datafusion.Column right_join_column = 6;
bool null_equals_null = 7;
LogicalExprNode filter = 8;
}

message UnionNode {
Expand Down
13 changes: 12 additions & 1 deletion datafusion/proto/src/logical_plan.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,11 @@ impl AsLogicalPlan for LogicalPlanNode {
join.join_constraint
))
})?;
let filter: Option<Expr> = join
.filter
.as_ref()
.map(|expr| parse_expr(expr, ctx))
.map_or(Ok(None), |v| v.map(Some))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe https://doc.rust-lang.org/std/option/enum.Option.html#method.transpose ? I'm having a hard time figuring out what the types are though so could be mistaken

Copy link
Contributor

@korowa korowa May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Transpose seems to be appropriate -- I used this map_or call in ballista for conversion from Option<Result> to Result<Option>

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied the code over from Ballista and am not too familiar with what is going on here. @korowa would you mind updating this in a follow on PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

NP 👌


let builder = LogicalPlanBuilder::from(into_logical_plan!(
join.left,
Expand All @@ -616,7 +621,7 @@ impl AsLogicalPlan for LogicalPlanNode {
&into_logical_plan!(join.right, ctx, extension_codec)?,
join_type.into(),
(left_keys, right_keys),
None, // filter
filter,
)?,
JoinConstraint::Using => builder.join_using(
&into_logical_plan!(join.right, ctx, extension_codec)?,
Expand Down Expand Up @@ -864,6 +869,7 @@ impl AsLogicalPlan for LogicalPlanNode {
left,
right,
on,
filter,
join_type,
join_constraint,
null_equals_null,
Expand All @@ -884,6 +890,10 @@ impl AsLogicalPlan for LogicalPlanNode {
let join_type: protobuf::JoinType = join_type.to_owned().into();
let join_constraint: protobuf::JoinConstraint =
join_constraint.to_owned().into();
let filter = filter
.as_ref()
.map(|e| e.try_into())
.map_or(Ok(None), |v| v.map(Some))?;
Ok(protobuf::LogicalPlanNode {
logical_plan_type: Some(LogicalPlanType::Join(Box::new(
protobuf::JoinNode {
Expand All @@ -894,6 +904,7 @@ impl AsLogicalPlan for LogicalPlanNode {
left_join_column,
right_join_column,
null_equals_null: *null_equals_null,
filter,
},
))),
})
Expand Down