Skip to content

Conversation

@HaoYang670
Copy link
Contributor

Which issue does this PR close?

Closes #5573.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: remzi <13716567376yh@gmail.com>
Signed-off-by: remzi <13716567376yh@gmail.com>
@github-actions github-actions bot added the logical-expr Logical plan and expressions label Mar 19, 2023
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.

Thank you @HaoYang670 -- this is a nice improvement

Comment on lines 638 to 641
let mut new_exprs = Vec::with_capacity(exprs.len());
for expr in exprs {
new_exprs.push(Self::replace_placeholders_with_values(expr, param_values)?);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could make this slightly more idiomatic like this (untested):

Suggested change
let mut new_exprs = Vec::with_capacity(exprs.len());
for expr in exprs {
new_exprs.push(Self::replace_placeholders_with_values(expr, param_values)?);
}
let new_exprs = exprs
.into_iter()
.map(|expr| Self::replace_placeholders_with_values(expr, param_values))
.collect::<Result<Vec<_>>()?;

Also, same thing below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catching @alamb ! I've updated the code, please review.

Signed-off-by: remzi <13716567376yh@gmail.com>
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.

Looks great @HaoYang670 -- thank you!

&self,
param_values: &Vec<ScalarValue>,
param_values: &[ScalarValue],
) -> Result<LogicalPlan, DataFusionError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps its good to reuse existing Result<LogicalPlan> which already Result<T, DataFusionError>. It will make the code less wordy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @comphead. Actually, I have noticed this, but as we always use Result<_, DatafusionError> in the file plan.rs, I didn't change it.

It is a good idea to file an issue to clean the Result<_, DatafusionError> usages in the project. But I think a more elegant way it to try to implement our own lint so that we can find it during the cargo clippy.

I've searched how to implement custom lints, but made less progress.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, I'll create a separate ticket to clean up. Lints are great ideas, not sure how easy to implement the custom ones

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @comphead. I will also file an issue to track the custom lint and find if there are more needs in our project to enable it.

Copy link
Contributor

Choose a reason for hiding this comment

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

#5644 for reference

@alamb
Copy link
Contributor

alamb commented Mar 20, 2023

Thanks @comphead and @HaoYang670

@alamb alamb merged commit c5c7ff4 into apache:main Mar 20, 2023
@alamb alamb added the api change Changes the API exposed to users of the crate label Mar 20, 2023
@HaoYang670 HaoYang670 deleted the 5573_replace_vector_with_slice branch March 21, 2023 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api change Changes the API exposed to users of the crate logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LogicalPlan::replace_params_with_values should not take &Vec<_>

3 participants