refactor: make apply_expression_roots more ergonomic

[jayshrivastava]

Which issue does this PR close?

• Follow up to Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) #24018 (review)

Rationale for this change

Allows us to rewrite

datafusion_physical_plan::apply_expression_roots(
    self.projection
        .source
        .iter()
        .map(|proj_expr| &proj_expr.expr),
    f,
)

as simply

datafusion_physical_plan::apply_expression_roots(self.projection.source.iter(), f)

What changes are included in this PR?

Adds a new trait and implements it for ProjectionExpr which facilitates the syntax above ^

pub trait PhysicalExprRoot {
    /// Returns the physical expression at this root.
    fn as_physical_expr_root(&self) -> &Arc<dyn PhysicalExpr>;
}

Are these changes tested?

Should be covered by existing coverage.

[jayshrivastava]

Ideally what we want is this

pub fn apply_expression_roots<I>(
    roots: I,
    f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
) -> Result<TreeNodeRecursion>
where
    I: IntoIterator,
    I::Item: AsRef<Arc<dyn PhysicalExpr>>,
{
    for root in roots {
        match f(root.as_ref())? {
            TreeNodeRecursion::Stop => return Ok(TreeNodeRecursion::Stop),
            TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {}
        }
    }
    Ok(TreeNodeRecursion::Continue)
}

However, AsRef<Arc<dyn PhysicalExpr>> is surprisingly implemented for Arc<dyn PhysicalExpr>. We cannot add this implementation due to the orphan rule. AsRef is foreign and Arc is foreign.

This PR gets around the problem by adding a new type, PhysicalExprRoot but that new type makes this refactor seem less worthwhile.

[jayshrivastava]

There is AsRef<dyn PhysicalExpr> for Arc<dyn PhysicalExpr> though but that requires changing the apply_expressions API to traverse over &dyn PhysicalExpr instead of &Arc<dyn PhysicalExpr>

[alamb]

I think as long as it is clear what the traits are for and it makes downstream code easier to copy/paste/ work it would be ok

[codecov-commenter]

Codecov Report

❌ Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.03%. Comparing base (a942c0b) to head (5d6146c).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/datasource-avro/src/source.rs	0.00%	1 Missing ⚠️
datafusion/datasource-json/src/source.rs	0.00%	1 Missing ⚠️
datafusion/physical-plan/src/execution_plan.rs	90.90%	0 Missing and 1 partial ⚠️
datafusion/proto/src/physical_plan/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24226      +/-   ##
==========================================
+ Coverage   80.99%   81.03%   +0.04%     
==========================================
  Files        1106     1107       +1     
  Lines      383330   384465    +1135     
  Branches   383330   384465    +1135     
==========================================
+ Hits       310467   311550    +1083     
- Misses      54545    54556      +11     
- Partials    18318    18359      +41     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[alamb]

maybe worth adding a comment here explaining what this does

[alamb]

I think as long as it is clear what the traits are for and it makes downstream code easier to copy/paste/ work it would be ok

[alamb]

it does look nicer

Though I admit perhaps the magic required to make it work reduces some of its value