zhuqi-lucas commented on code in PR #24445:
URL: https://github.com/apache/datafusion/pull/24445#discussion_r3826972346
##########
datafusion/physical-expr/src/equivalence/properties/mod.rs:
##########
@@ -1169,6 +1169,23 @@ impl EquivalenceProperties {
/// `output_schema`.
pub fn project(&self, mapping: &ProjectionMapping, output_schema:
SchemaRef) -> Self {
let eq_group = self.eq_group.project(mapping);
+ self.project_with_eq_group(mapping, output_schema, eq_group)
+ }
+
+ /// Same as [`Self::project`], but takes an already-projected equivalence
+ /// group instead of computing one.
+ ///
+ /// [`EquivalenceGroup::project`] is a pure function of the group and the
+ /// mapping, so a caller that knows both are unchanged since the last
+ /// projection can hand back the previous result rather than recomputing an
+ /// identical one. Orderings are still derived here: they are precisely
what
+ /// changes when a sort is introduced below this node.
+ pub fn project_with_eq_group(
Review Comment:
Done, that shape is better and it removes the problem rather than asserting
around it. `project_reusing` now takes `previous` and `cached`, compares the
groups itself and falls back to a full projection, so the `debug_assert` and
the unchecked entry point it needed are both gone.
One knock-on worth flagging: the decision moving into `physical-expr` means
`ProjectionExec` no longer knows whether reuse happened, and `cfg(test)` does
not reach across crates, so the counter you asked for earlier could not stay
where it was. The tests split by what they prove — `physical-expr` asserts the
reuse fired, `physical-plan` asserts the resulting properties are right.
Disabling the reuse still fails the former.
I did not need `projected_from`: since the signature takes `previous`, the
comparison has both sides already.
On the `Arc` — you are right and my stated reason was wrong; I checked your
three points and they hold, so I have corrected the description. Keeping it
separate for a different reason: `Arc::make_mut` adds a cost at every mutation,
and `ptr_eq` changes *which* cases hit rather than only how fast the check is,
so it wants its own before/after rather than riding along in a PR whose numbers
are already settled. Happy to do it next, or here if you would rather see them
together.
--
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]
