andygrove commented on PR #4827: URL: https://github.com/apache/datafusion-comet/pull/4827#issuecomment-5876362820
This is a light fully automated review since there are so many PRs open. 1. The hook at `QueryPlanSerde.scala:900` also fires when a node was declined only because the user turned it off with `spark.comet.expression.<Name>.enabled=false` (the check at line 814). `docs/source/user-guide/latest/compatibility/regex.md` documents that flag as forcing a Spark fallback, useful for narrowing a regression. With `spark.comet.exec.jvmDetour.enabled=true` it no longer does that. The default path in `CometRLike.convert` (`strings.scala:439`) is already `emitJvmCodegenDispatch`, so with `spark.comet.expression.RLike.enabled=false` the hook emits the same `JvmScalarUdf` and the switch does nothing. The same holds for every `CometCodegenDispatch` serde (the higher-order functions, `CreateMap`, the date/time set). For native serdes like `Abs` it swaps one Comet path for another instead of falling back. Could the hook refuse any subtree that contains an explicitly disabled node, not just the node itself, since an ancestor's detour would carry it along? The suite, the f uzz tests and the benchmark would then need another way to push a node through the hook, for example a genuinely unsupported expression like `sentences` or a `testing`-category config. 2. The new guard at `CometScalaUDF.scala:99` explains that arg 0 is serialized at plan time, before `waitForSubqueries` runs, which matches how the proto is built. But the comment at `CometBatchKernelCodegen.scala:146-150` still says subqueries are accepted because `waitForSubqueries` populates the result before the closure serializer captures it, and the comment on the `CometCodegenSuite` test at line 741 says the same. Could this PR update both, so nobody later takes the old comment as the contract and removes the guard? -- 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]
