mingmwang commented on code in PR #5322:
URL: https://github.com/apache/arrow-datafusion/pull/5322#discussion_r1119528195
##########
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:
Regarding the method `apply_subrules_and_check_finiteness_requirements()`
I would suggest to make the `input` immutable. If there is changes to the
plan/struct, return the changed (new)plan/struct.
```rust
fn apply_subrules_and_check_finiteness_requirements(
mut input: PipelineStatePropagator,
physical_optimizer_subrules: &Vec<Box<PipelineFixerSubrule>>,
)
```
Suggest change to
```rust
fn apply_subrules_and_check_finiteness_requirements(
input: PipelineStatePropagator,
physical_optimizer_subrules: &[Box<PipelineFixerSubrule>],
) -> Result<Option<PipelineStatePropagator>> {
let after_op =
physical_optimizer_subrules
.iter()
.try_fold(input, |pipeline, sub_rule| {
if let Some(value) = sub_rule(&pipeline).transpose()? {
Result::<_, DataFusionError>::Ok(value)
} else {
Result::<_, DataFusionError>::Ok(pipeline)
}
})?;
check_finiteness_requirements(after_op)
}
```
--
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
