ozankabak commented on code in PR #5322:
URL: https://github.com/apache/arrow-datafusion/pull/5322#discussion_r1119610748
##########
datafusion/core/src/physical_optimizer/pipeline_fixer.rs:
##########
@@ -182,13 +289,46 @@ fn apply_subrules_and_check_finiteness_requirements(
physical_optimizer_subrules: &Vec<Box<PipelineFixerSubrule>>,
) -> Result<Option<PipelineStatePropagator>> {
for sub_rule in physical_optimizer_subrules {
- if let Some(value) = sub_rule(&input).transpose()? {
+ if let Some(value) = sub_rule(input.clone()).transpose()? {
input = value;
}
}
check_finiteness_requirements(input)
}
Review Comment:
I tried it, but this creates an ownership problem in the closure (since
`sub_rule` does not take a reference as its argument). We can make this work if
we change the sub-rule return type so that it always returns an
`PipelineStatePropagator` (by returning the argument unchanged when nothing is
modified), and not an `Option`. Maybe we can explore this in a refactor PR.
BTW, when we were investigating `clone` calls, we came across a similar
situation in some use cases involving `transform_up`/`transform_down`. Maybe we
can consider all of those in that PR and discuss together the details. Will
keep this in mind, thanks for pointing it out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
