peter-toth commented on code in PR #58999: URL: https://github.com/apache/spark/pull/58999#discussion_r4091210332
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CallMethodViaReflection.scala: ########## @@ -216,6 +216,11 @@ case class CallMethodViaReflection( /** A temporary buffer used to hold intermediate results returned by children. */ @transient private lazy val buffer = new Array[Object](argExprs.length) + // Stateful because of the reusable `buffer` holding the reflection arguments: it is filled by + // `evalInternal` and then handed to the call, so two evaluations sharing one instance would + // overwrite each other's arguments. Fresh copies give every evaluation its own buffer. + override def stateful: Boolean = true Review Comment: **Finding 1.** This flag is right here. The same flag on `XPathExtract` is now effective too, which is what changed since #57371. My review there argued the XPath hunk was dead because `ReplaceExpressions` sits in `FinishAnalysis`, the optimizer's first batch, so every `XPathExtract` becomes `Invoke(Literal(evaluator), ...)` before any of the four `freshCopyIfContainsStatefulExpression()` callers run. That was true on 2026-08-12. [SPARK-58208](https://github.com/apache/spark/commit/73d3dbcf71acbb6fecb15b4480462543eb1bff20) then landed on 2026-08-22 and added a fifth caller, `QueryExecution.cloneWithFreshStatefulExpressions` (`sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala:338`), which runs on the **analyzed** plan. `XPathExtract` is still itself at that point, and `replacement` is a `def` that reads the new instance's `@transient lazy val evaluator`, so a fresh copy really does get a fresh `UDFXPathUtil`. I measured it on your head commit. Two sibling copies, their `replacement` evaluated concurrently on two threads with a matching and a non-matching document, 2000 iterations each: ``` // as-is on this PR's head PROBE stateful=false, c1 eq c2 = true PROBE wrongResults=2 exceptions=3904 (of 4000 evals) // with `override def stateful: Boolean = true` added to XPathExtract PROBE stateful=true, c1 eq c2 = false PROBE wrongResults=0 exceptions=0 (of 4000 evals) ``` The exceptions are the shared `ReusableStringReader` and `DocumentBuilder` inside `UDFXPathUtil`, which is documented as non-thread-safe ("Each UDF instance should keep an instance of this class", `sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/xml/UDFXPathUtil.java:40`). The one-line hunk is: ```scala protected def evaluator: XPathEvaluator override def stateful: Boolean = true ``` `catalyst/testOnly org.apache.spark.sql.catalyst.expressions.xml.*` stays green with it (20 tests). Whichever way you go, please don't narrow the JIRA title yet. Once SPARK-58209 says `CallMethodViaReflection` only, the XPath half has no ticket. -- 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]
