zhuqi-lucas commented on code in PR #24445:
URL: https://github.com/apache/datafusion/pull/24445#discussion_r3823023448
##########
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:
Thanks @adriangb , very good suggestion!
Done, and it reads much better. `compute_properties` takes the optional
group, `try_from_projector` delegates with `None`, and there is one body
instead of two. `replace_children` loses the early return and the second call
site as you sketched.
Left the guard in `ProjectionExec` rather than moving it into
`EquivalenceProperties`. The soundness argument leans on facts the plan node
owns -- that the projector was carried over untouched, so the mapping is
unchanged -- and the `(previous_group, produced_group)` pair reads as a cache
key that `EquivalenceProperties` would have no way to validate. Happy to move
it if you would rather it live there.
No behaviour change; deleting the fast path still fails the two tests that
assert it is taken.
--
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]
