adriangb commented on code in PR #24445:
URL: https://github.com/apache/datafusion/pull/24445#discussion_r3822873955
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -194,6 +219,55 @@ impl ProjectionExec {
})
}
+ /// Like [`Self::try_from_projector`], but reuses `eq_group` as the output
+ /// equivalence group instead of projecting the input's group again.
+ ///
+ /// [`EquivalenceGroup::project`] is a pure function of the group and the
+ /// mapping, so reuse is sound exactly when both are unchanged.
+ ///
+ /// The caller establishes the first by comparing the old and new child
+ /// groups. The second holds because the mapping comes from
+ /// `projector.projection()`, carried over untouched, and from the child's
+ /// schema, which `ProjectionMapping::try_new` consults only for field
names
+ /// and indices -- never for types or nullability. So a child differing
only
+ /// in nullability keeps the same mapping. A child that renamed or
reordered
+ /// those fields would change the group too, since its members are
`Column`s
+ /// carrying those names, and the comparison above would reject it; were
one
+ /// to slip through anyway, `try_new`'s name assertion errors out rather
than
+ /// letting a stale group into the plan.
+ fn try_from_projector_reusing_eq_group(
+ projector: Projector,
+ input: Arc<dyn ExecutionPlan>,
+ eq_group: EquivalenceGroup,
+ ) -> Result<Self> {
Review Comment:
This duplicates `compute_properties` and can drift if `PlanProperties` grows
a new field.
The duplication exists only because reuse is expressed as a second
constructor. But the two paths differ in exactly one expression — how
eq_properties is obtained. Everything else (output_partitioning,
pipeline_behavior, boundedness, PlanProperties::new, the Self { … } literal) is
copied. So push the difference down into compute_properties as a parameter and
delete the second constructor:
```rust
fn compute_properties(
input: &Arc<dyn ExecutionPlan>,
projection_mapping: &ProjectionMapping,
schema: SchemaRef,
reused_eq_group: Option<EquivalenceGroup>, // <- the only new knob
) -> Result<PlanProperties> {
let input_eq_properties = input.equivalence_properties();
// The only thing reuse changes. Everything below is common, so the two
// paths cannot drift.
let eq_properties = match reused_eq_group {
Some(eq_group) => input_eq_properties
.project_with_eq_group(projection_mapping, schema, eq_group),
None => input_eq_properties.project(projection_mapping, schema),
};
// ... unchanged: partitioning, PlanProperties::new
}
fn try_from_projector(projector, input) -> Result<Self> {
Self::try_from_projector_with_eq_group(projector, input, None)
}
fn try_from_projector_with_eq_group(
projector: Projector,
input: Arc<dyn ExecutionPlan>,
reused_eq_group: Option<EquivalenceGroup>,
) -> Result<Self> { /* the single existing body */ }
```
`replace_children(Recompute)` then loses its early return and its duplicated
call. The guard just produces the Option:
```rust
let child = children.swap_remove(0);
let reused_eq_group = if self.input.equivalence_properties().eq_group()
.has_same_classes(child.equivalence_properties().eq_group())
{
#[cfg(test)]
eq_group_reuse_probe::record_hit();
Some(self.cache.equivalence_properties().eq_group().clone())
} else {
None
};
ProjectionExec::try_from_projector_with_eq_group(self.projector.clone(),
child, reused_eq_group)
.map(|p| Arc::new(p) as _)
```
We could also move the guard into EquivalenceProperties instead of the group:
```rust
pub fn project_reusing(
&self,
mapping: &ProjectionMapping,
output_schema: SchemaRef,
// (the eq properties this was last projected from, the group that
produced)
previous: Option<(&EquivalenceGroup, &EquivalenceGroup)>,
) -> Self
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
