cloud-fan commented on PR #52956:
URL: https://github.com/apache/spark/pull/52956#issuecomment-3981734173
*Disclaimer: This review was generated by Cursor with Claude Opus 4.6.*
The overall approach makes sense — cloning the lambda via
`LambdaMetafactory` instead of mutating `arg$1` on hidden classes is the right
way to handle JEP 416 on Java 22+. A few issues I noticed:
**1. Typo: `makeClonedIndyLambdaFacory` → `makeClonedIndyLambdaFactory`**
Missing 't' in the method name.
**2. Typos in Scaladoc for `getFullPowerLookupFor`**
- "crated at each REPL line" → "created at each REPL line"
- "a lookup which doesn't enough privilege" → "a lookup which doesn't have
enough privilege"
**3. No fallback if `cloneIndyLambda` fails**
If `makeClonedIndyLambdaFactory` or `factory.invokeWithArguments` throws
(e.g., unexpected lambda shape, classloader issue, or future JDK change), the
exception propagates uncaught. Since this is a new code path that's mandatory
on Java 22+, consider wrapping it in a `try-catch` that provides a clear error
message, or at least logs a warning pointing users to the
`SPARK_CLONE_BASED_CLOSURE_CLEANER` env var / system property as a
troubleshooting hint.
**4. `StateSpec.scala`: potentially unnecessary `asInstanceOf` casts**
```scala
val cleanedMappingFunction =
SparkClosureCleaner.clean(mappingFunction, checkSerializable = true)
.asInstanceOf[(Time, KeyType, Option[ValueType], State[StateType]) =>
Option[MappedType]]
```
`SparkClosureCleaner.clean[F <: AnyRef]` is generic and returns `F`, so
Scala should infer the correct type without `asInstanceOf`. If type inference
fails here, an explicit type parameter would be safer than an unchecked cast.
**5. Mixed semantics of `clean` return value**
The return type changed from `Boolean` to `Option[F]`, but for
non-indylambda closures and the old mutation path (Java < 22), the method still
mutates in place and returns `Some(func)` (same reference). On the clone path
(Java 22+), it returns a genuinely new object. All call sites handle this
correctly, but a brief doc comment on the `clean` method explaining this dual
behavior would help future maintainers.
---
_This comment was generated with [GitHub MCP](http://go/mcps)._
--
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]
