refactor: make apply_expression_roots more ergonomic - #24226
refactor: make apply_expression_roots more ergonomic#24226jayshrivastava wants to merge 1 commit into
Conversation
github-actions
Bot
added
physical-expr
| /// [`TreeNodeRecursion::Stop`] stops iteration and is returned immediately. | ||
| /// [`TreeNodeRecursion::Jump`] is normalized to [`TreeNodeRecursion::Continue`] | ||
| /// because this function does not visit expression children. | ||
| pub fn apply_expression_roots<I>( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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 Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24226 +/- ##
=======================================
Coverage 80.98% 80.99%
=======================================
Files 1106 1106
Lines 383232 383215 -17
Branches 383232 383215 -17
=======================================
- Hits 310372 310368 -4
+ Misses 54543 54525 -18
- Partials 18317 18322 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| impl Eq for ProjectionExpr {} | ||
|
|
||
| impl AsRef<Arc<dyn PhysicalExpr>> for ProjectionExpr { |
There was a problem hiding this comment.
maybe worth adding a comment here explaining what this does
| /// [`TreeNodeRecursion::Stop`] stops iteration and is returned immediately. | ||
| /// [`TreeNodeRecursion::Jump`] is normalized to [`TreeNodeRecursion::Continue`] | ||
| /// because this function does not visit expression children. | ||
| pub fn apply_expression_roots<I>( |
There was a problem hiding this comment.
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
| .map(|proj_expr| &proj_expr.expr), | ||
| f, | ||
| ) | ||
| crate::apply_expression_roots(self.projector.projection().as_ref().iter(), f) |
There was a problem hiding this comment.
it does look nicer
Though I admit perhaps the magic required to make it work reduces some of its value
3 participants
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?