gstvg commented on code in PR #22853: URL: https://github.com/apache/datafusion/pull/22853#discussion_r3527236493
########## datafusion/expr/src/higher_order_function.rs: ########## Review Comment: Yes, is dynamic and require walking the body, but at least for this PR, it's folded with the walk that collect used indices. > is it actually possible to build a LambdaArgument like as an api? Technically yes but not because I imagined a usecase for it outside datafusion, but only to be able to use it in `evaluate` which is within another crate Some other ideas besides https://github.com/apache/datafusion/pull/22853#issuecomment-4662954376: 1) Providing the `LambdaExpr` itself (instead of it's body) as the `Arc<dyn PhyiscalExpr>` parameter of `LambdaArgument::new`, so `evaluate` can get the used params by downcasting the expr to `LambdaExpr` (or returning an error if it fails) 2) computing the lambda used params in `LambdaArgument::evaluate`, at the small cost of a tree traversal per evaluation instead of only during planning ########## datafusion/physical-expr/src/expressions/lambda.rs: ########## @@ -129,6 +141,7 @@ impl LambdaExpr { body, projected_body, projection, + used_params: used_param_names, Review Comment: The initial version of this PR https://github.com/apache/datafusion/commit/b9f816f70900bbc2ef9e1c719018e3f73e6f0bf0 did that, but introduced a breaking change by adding a new argument `outer_columns_count` in `LambdaExpr::try_new`. I then proposed non-breaking alternatives https://github.com/apache/datafusion/pull/22853#issuecomment-4662954376 (which is self-contained but uses null arrays to avoid uncaptured columns copies) and https://github.com/apache/datafusion/pull/22853#issuecomment-4675481540 (the current version, which has the small advantage of skipping the evaluation of declared but unused parameters, for example `(x, y) -> x+1`). Another self-contained fix that stills uses projection, and maybe also skip unused params would be great. Using the first suggestion is fine by me as well. WDYT? -- 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]
