andygrove commented on PR #4744: URL: https://github.com/apache/datafusion-comet/pull/4744#issuecomment-5876433655
This is a light fully automated review since there are so many PRs open. `docs/source/contributor-guide/roadmap.md:48-50` still describes DataFusion's `array_filter` as native higher-order function support "that Comet does not yet use". Once this lands with `spark.comet.exec.higherOrderFunction.native.enabled` defaulting to `true`, unary `filter` lambdas run through that function by default, so the "Native Lambda Evaluation" section becomes wrong. Could that paragraph be updated in this PR? A few code comments also describe code that no longer exists. Item 3 of the module doc at `native/core/src/execution/lambda.rs:28-30` describes a wrapper that keeps unused lambda parameters visible in `children()`, which went away when DataFusion 55's `LambdaExpr` started computing `used_param_indices()` itself. The module doc also never mentions `EmptyBatchGuardExpr` or `ShortCircuitBinaryExpr`, which are what the file contains now. `native/core/src/execution/planner.rs:3850` says "the guard pops on any `?` / drop", but `with_scope` pops explicitly and there is no guard anymore. The Scaladoc at `spark/src/main/scala/org/apache/comet/serde/CometHighOrderFunction.scala:169` points at `StrictBooleanExpr`, which should be `ShortCircuitBinaryExpr`. -- 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]
