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

Remove parse_vec_expr helper #12178

Merged

Conversation

findepi
Copy link
Member

@findepi findepi commented Aug 26, 2024

Behavior of parse_vec_expr and parse_exprs is almost similar -- both take a collection of expressions to parse. The only difference is that parse_vec_expr returns Option::None when collections is empty, but this difference in behavior does not correspond to difference in function names. Since the function is used once only, remove it instead of coming up with a fancy name.

Prep for #12177

@github-actions github-actions bot added the proto Related to proto crate label Aug 26, 2024
Behavior of `parse_vec_expr` and `parse_exprs` is almost similar -- both
take a collection of expressions to parse. The only difference is that
`parse_vec_expr` returns `Option::None` when collections is empty, but
this difference in behavior does not correspond to difference in
function names. Since the function is used once only, remove it instead
of coming up with a fancy name.
@findepi findepi force-pushed the findepi/remove-parse-vec-expr-helper-db889e branch from 4661698 to e09f43d Compare August 26, 2024 16:18
Comment on lines +589 to +592
match pb.order_by.len() {
0 => None,
_ => Some(parse_exprs(&pb.order_by, registry, codec)?),
},
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

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

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Seems like this is a reasonable change to me and we are tracking futher improvements. Let's keep the code flowing

Thank you @findepi and @crepererum for the review

@alamb alamb merged commit a616e88 into apache:main Aug 27, 2024
25 checks passed
@findepi findepi deleted the findepi/remove-parse-vec-expr-helper-db889e branch August 27, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proto Related to proto crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants