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

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

Merged
merged 3 commits into from
Mar 20, 2023
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac16311
update the function type
HaoYang670 Mar 19, 2023
a730d71
merge upstream/main
HaoYang670 Mar 19, 2023
4a861e8
address comment
HaoYang670 Mar 19, 2023
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
34 changes: 15 additions & 19 deletions datafusion/expr/src/logical_plan/plan.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@ use crate::logical_plan::display::{GraphvizVisitor, IndentVisitor};
use crate::logical_plan::extension::UserDefinedLogicalNode;
use crate::logical_plan::plan;
use crate::utils::{
self, exprlist_to_fields, from_plan, grouping_set_expr_count,
grouping_set_to_exprlist,
exprlist_to_fields, from_plan, grouping_set_expr_count, grouping_set_to_exprlist,
};
use crate::{
build_join_schema, Expr, ExprSchemable, TableProviderFilterPushDown, TableSource,
Expand Down Expand Up @@ -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

let exprs = self.expressions();
let mut new_exprs = vec![];
for expr in exprs {
new_exprs.push(Self::replace_placeholders_with_values(expr, param_values)?);
}
let new_exprs = self
.expressions()
.into_iter()
.map(|e| Self::replace_placeholders_with_values(e, param_values))
.collect::<Result<Vec<_>, DataFusionError>>()?;

let new_inputs = self.inputs();
let mut new_inputs_with_values = vec![];
for input in new_inputs {
new_inputs_with_values.push(input.replace_params_with_values(param_values)?);
}
let new_inputs_with_values = self
.inputs()
.into_iter()
.map(|inp| inp.replace_params_with_values(param_values))
.collect::<Result<Vec<_>, DataFusionError>>()?;

let new_plan = utils::from_plan(self, &new_exprs, &new_inputs_with_values)?;
Ok(new_plan)
from_plan(self, &new_exprs, &new_inputs_with_values)
}

/// Walk the logical plan, find any `PlaceHolder` tokens, and return a map of their IDs and DataTypes
Expand Down Expand Up @@ -748,10 +746,8 @@ impl LogicalPlan {
Ok(Expr::Literal(value.clone()))
}
Expr::ScalarSubquery(qry) => {
let subquery = Arc::new(
qry.subquery
.replace_params_with_values(&param_values.to_vec())?,
);
let subquery =
Arc::new(qry.subquery.replace_params_with_values(param_values)?);
Ok(Expr::ScalarSubquery(plan::Subquery { subquery }))
}
_ => Ok(expr),
Expand Down