Raviston6296 commented on PR #60170: URL: https://github.com/apache/spark/pull/60170#issuecomment-6685821389
Thanks for tightening this. Since the description says the focused SBT run wasn't possible locally, I ran it on the PR head (f6254062261) with JDK 21: - `connect/testOnly org.apache.spark.sql.connect.ml.MLClassLoadingSuite`: 4/4 pass. - With the SPARK-60608 check reverted (`MLAllowListedLoader.load` back to `Utils.classForName` in `DefaultParamsReader.loadParamsInstanceReader`), only `reject a non-allowlisted nested stage before invoking its reader` fails: `Expected exception ... MlUnsupportedException to be thrown, but no exception was thrown`. The other three still pass. That matches the description. With the old `DefaultParamsReadable` fixture, `DefaultParamsReader.load` calls `MLAllowListedLoader.load` itself, so it rejected the stage whether or not `loadParamsInstanceReader` did. The custom reader removes that second check, and `readCalls` pins that the rejection happens before `read` is invoked. LGTM. One note in case this goes to the maintenance branches: SPARK-60608 is on branch-4.0 through branch-4.x. On branch-4.0 the reject test intercepts `InvocationTargetException` and checks its cause (a335a9e723c), because branch-4.0's `MLUtils.loadOperator` does not unwrap it. So this change won't cherry-pick cleanly there. I'm happy to open the branch-4.0 backport if it's wanted. -- 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]
