LiaCastaneda commented on code in PR #22853:
URL: https://github.com/apache/datafusion/pull/22853#discussion_r3530029555
##########
datafusion/physical-expr/src/expressions/lambda.rs:
##########
@@ -129,6 +141,7 @@ impl LambdaExpr {
body,
projected_body,
projection,
+ used_params: used_param_names,
Review Comment:
ok thanks for explaining, so the correctness fix can be self contained in
`datafusion-physical-expr` with no new public API, but the optimization of
skipping unused param evaluation requires crossing into `datafusion-expr`,
which forces `new_with_used_params` to be public (even if it's only meant to be
used internally).
My approach is non-breaking but does not skip unused param evaluation. I'm
not sure this is achievable while keeping the fix self contained in
`datafusion-physical-expr`, since the `evaluate` lives in `datafusion-expr`.
I think the optimization (therefore this approach) is worth keeping, the
cost of evaluating the unused param will grow with the number of map entries,
so for large inputs skipping an unused param closure might be meaningful
--
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]
