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

Update the type of param_values to &[ScalarValue] in function replace_params_with_values #5640

Merged
merged 3 commits into from
Mar 20, 2023

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!

@@ -632,22 +631,21 @@ impl LogicalPlan {
/// params_values
pub fn replace_params_with_values(
&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