Copilot commented on code in PR #20239:
URL: https://github.com/apache/datafusion/pull/20239#discussion_r2782625238
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -1310,10 +1311,19 @@ fn rewrite_projection(
})
.partition(|(_, value)| value.is_volatile());
+ // Partition non-volatile expressions by MoveTowardsLeafNodes vs
everything else.
+ // MoveTowardsLeafNodes expressions (like get_field) are cheap — no
benefit to
+ // re-inlining them into filters, and it causes optimizer instability with
+ // ExtractLeafExpressions.
+ let (move_towards_leaves_map, pushable_map): (HashMap<_, _>, HashMap<_,
_>) =
+ non_volatile_map.into_iter().partition(|(_, value)| {
+ value.placement() == ExpressionPlacement::MoveTowardsLeafNodes
+ });
Review Comment:
The new partitioning uses `value.placement() ==
ExpressionPlacement::MoveTowardsLeafNodes`, but the PR title/intent references
skipping pushdown when `placement().should_push_to_leaves()` is true (which
also includes `Column`). Please either (a) switch to
`value.placement().should_push_to_leaves()` if the intent is to cover both
`Column` and `MoveTowardsLeafNodes`, or (b) adjust the surrounding comment/PR
messaging to clarify this special-casing is only for `MoveTowardsLeafNodes`
expressions.
--
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]
