Skip to content

Remove parse_vec_expr helper #12178

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

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Remove parse_vec_expr helper #12178

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e09f43d
Remove `parse_vec_expr` helper
findepi Aug 26, 2024
925dae8
empty
findepi Aug 26, 2024
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
15 changes: 4 additions & 11 deletions datafusion/proto/src/logical_plan/from_proto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,10 @@ pub fn parse_expr(
parse_exprs(&pb.args, registry, codec)?,
pb.distinct,
parse_optional_expr(pb.filter.as_deref(), registry, codec)?.map(Box::new),
parse_vec_expr(&pb.order_by, registry, codec)?,
match pb.order_by.len() {
0 => None,
_ => Some(parse_exprs(&pb.order_by, registry, codec)?),
},
Comment on lines +589 to +592
Copy link
Member Author

Choose a reason for hiding this comment

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

this pattern is already used here

datafusion/datafusion/proto/src/logical_plan/mod.rs

Lines 817 to 819 in 117ab1b

let sort_expr = match distinct_on.sort_expr.len() {
0 => None,
_ => Some(

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if in both places, we could use another new helper like

fn optimize_empty_vec<T>(v: Vec<T>) -> Option<Vec<T>> {
    (!v.empty()).then_some(v)
}

so the combination would look like this:

optimize_empty_vec(parse_exprs(&pb.order_by, registry, codec)?)

However, that's a personal preference, mostly based on the fact that naming the input (pb.order_by) twice, which can easily introduce copy&paste errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

empty_vec_to_none? vec_if_non_empty?

in this case, however, my preference would be to remove Option and keep just Vec.
Empty collection perfectly describes lack of sorting, so no need to wrap if with optional value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty collection perfectly describes lack of sorting, so no need to wrap if with optional value.

Let's try to do that then. I agree that having an Option<Vec<_>> in new_udf doesn't make much sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea. i would rather have this as a follow-up though.
this work would conflict with #12177 considerably.

Copy link
Member Author

Choose a reason for hiding this comment

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

filed #12195 issue for this

None,
)))
}
Expand Down Expand Up @@ -676,16 +679,6 @@ pub fn from_proto_binary_op(op: &str) -> Result<Operator, Error> {
}
}

fn parse_vec_expr(
p: &[protobuf::LogicalExprNode],
registry: &dyn FunctionRegistry,
codec: &dyn LogicalExtensionCodec,
) -> Result<Option<Vec<Expr>>, Error> {
let res = parse_exprs(p, registry, codec)?;
// Convert empty vector to None.
Ok((!res.is_empty()).then_some(res))
}

fn parse_optional_expr(
p: Option<&protobuf::LogicalExprNode>,
registry: &dyn FunctionRegistry,
Expand Down