Remove expr_sub_expressions and rewrite_expression functions

[mrob95]

Which issue does this PR close?

Follows from #2736 and #2765

Rationale for this change

These functions were redundant with ExprVisitor and ExprRewriter and were error prone (as demonstrated in #2736).

What changes are included in this PR?

Replaced all users of these functions with a visitor or a rewriter as necessary, and deleted them.

Something I'm unsure of: I feel like I'm using Expr.clone all over the place - for example in filter_push_down at the call sites of replace_cols_by_name - but I don't have enough rust experience to know if this is a problem or not..

Are there any user-facing changes?

[codecov-commenter]

Codecov Report

Merging #2772 (1bcd9c0) into master (748b6a6) will increase coverage by 0.20%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##           master    #2772      +/-   ##
==========================================
+ Coverage   84.95%   85.15%   +0.20%     
==========================================
  Files         271      272       +1     
  Lines       48164    48073      -91     
==========================================
+ Hits        40916    40938      +22     
+ Misses       7248     7135     -113     

Impacted Files	Coverage Δ
datafusion/optimizer/src/utils.rs	88.57% <ø> (+53.00%)	⬆️
datafusion/core/src/physical_optimizer/pruning.rs	93.75% <83.33%> (-0.03%)	⬇️
datafusion/optimizer/src/filter_push_down.rs	98.23% <100.00%> (ø)
...atafusion/optimizer/src/subquery_filter_to_join.rs	93.86% <100.00%> (+0.53%)	⬆️
datafusion/optimizer/src/filter_null_join_keys.rs	91.83% <0.00%> (-2.05%)	⬇️
datafusion/core/src/physical_plan/metrics/value.rs	86.93% <0.00%> (-0.51%)	⬇️
datafusion/core/src/execution/context.rs	78.37% <0.00%> (-0.28%)	⬇️
datafusion/expr/src/logical_plan/plan.rs	74.11% <0.00%> (-0.20%)	⬇️
datafusion/core/src/config.rs	91.11% <0.00%> (ø)
datafusion/core/src/physical_plan/hash_join.rs	94.68% <0.00%> (+0.57%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 748b6a6...1bcd9c0. Read the comment docs.

[andygrove]

LGTM. I left one code suggestion. Would be good to get a review from @alamb as well before we merge.

Thanks @mrob95

[alamb]

This looks great -- what a nice cleanup. Thank you @mrob95 !

Something I'm unsure of: I feel like I'm using Expr.clone all over the place - for example in filter_push_down at the call sites of replace_cols_by_name - but I don't have enough rust experience to know if this is a problem or not..

I think this is a consequence of the fact that the optimizer passes create new LogicalPlans (rather than rewriting the existing ones). Maybe we can work to improve this over time but it is tricky as the logical plans are sometimes wrapped in Arcs (and can be shared).

I don't think this PR adds any new copies, but it makes the cloning more obvious (previously expr_sub_expression and rewrite_expression internally made all the clones.

[alamb]

🔥 very nice