andygrove commented on PR #4744:
URL:
https://github.com/apache/datafusion-comet/pull/4744#issuecomment-5441809233
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
Moving lambda evaluation into DataFusion instead of paying a JNI call per
batch is clearly the right direction, and the design here is thoughtful:
resolving `NamedLambdaVariable` by Spark `exprId` rather than by name sidesteps
shadowing, and the wrapper that keeps unused lambda parameters visible in
`children()` so projection compaction stays consistent with the runtime layout
is a subtle detail well handled.
Four things.
**The configs are read once per JVM, not per session**
```scala
case class CometHighOrderFunction[T <: HigherOrderFunction](name: String)
extends CometExpressionSerde[T] {
private val nativeHofEnabled =
CometConf.COMET_EXEC_HIGHER_ORDER_FUNCTION_NATIVE_ENABLED.get()
private val codegenEnabled =
CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.get()
```
`CometArrayFilter` is an `object` extending this case class, so both `val`s
are evaluated once when the object is first initialized, and never again. That
means `spark.comet.exec.higherOrderFunction.native.enabled` is effectively
frozen at whatever it was on first use, and `withSQLConf(...)` in a test or a
per-session override in production will not take effect.
Other serdes read configs inside `getSupportLevel` / `convert` precisely for
this reason. Could these become `def`s, or be read from the `conf` passed into
the conversion? This is also going to make the feature untestable with
`withSQLConf`, which is worth checking against the existing tests: if a test
appears to exercise both paths, it may only be exercising whichever ran first.
**Defaulting the native path to `true`**
A new native lambda execution path with a scope stack, exprId resolution,
and projection compaction, enabled by default in its first release. Given how
much new machinery this is, would `false` be safer for one release? The
fallback chain (native, then dispatcher, then Spark) is already in place, so
users who want it can opt in and the risk of a subtle lambda-scoping bug
reaching everyone is much lower.
**The module doc no longer matches `with_scope`**
`lambda.rs`'s header says the planner needs "(2) A drop-guard that pops a
scope on any exit path (`?`, panic-safe)", but `with_scope`'s own comment says
"The pop happens on both the `Ok` and `Err` paths, this replaces the earlier
RAII guard". `with_scope` is not panic-safe: a panic inside `f` leaves the
scope on the stack.
Since `PhysicalPlanner` catches panics across the FFI boundary and could in
principle keep planning, that could leave a stale scope that resolves a
variable it should not. Either restore the guard or fix the header so it does
not promise panic safety it no longer provides.
**No performance number**
The stated motivation is removing a per-batch JNI call. What does that buy
for `array_filter` over a realistic batch? A microbenchmark or an end-to-end
comparison against the dispatcher path would justify the complexity, and would
also tell reviewers whether extending this to `transform`, `exists`, and
`aggregate` is worth doing.
--
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]
