HyukjinKwon commented on code in PR #57354:
URL: https://github.com/apache/spark/pull/57354#discussion_r3611561843
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -585,6 +590,8 @@ case class SchemaOfJson(
}
}
+ override def stateful: Boolean = true
Review Comment:
`SchemaOfJson` is `RuntimeReplaceable` and requires a **foldable** child
(`NON_FOLDABLE_INPUT` check above), and `SchemaOfJsonEvaluator` holds only
config lazy-vals (no per-row `var`). A foldable input is constant-folded via a
single eval, so no multi-eval race is possible — this override is unnecessary.
It's also inconsistent with `SchemaOfXml` (same `RuntimeReplaceable` +
config-only-evaluator shape), which is correctly left unmarked. Suggest
dropping it (and the matching one on `SchemaOfCsv`).
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -518,6 +521,8 @@ case class StructsToJson(
override protected def withNewChildInternal(newChild: Expression):
StructsToJson =
copy(child = newChild)
+ override def stateful: Boolean = true
Review Comment:
`StructsToJson` is `RuntimeReplaceable`, so this override is dead at
execution. `ReplaceExpressions` (Finish Analysis batch, `Optimizer.scala:338`)
rewrites this node to `Invoke(Literal(evaluator), ...)` before any
execution-time `freshCopyIfContainsStatefulExpression`, and
`RuntimeReplaceable.eval` asserts `input == null` (`Expression.scala:462`) so
it's never row-evaluated in place. After replacement the evaluator is pinned
inside a `Literal` (non-stateful `LeafExpression`), and `freshCopy` on a
`Literal` returns `this` — both copies keep the same evaluator. So if a
shared-node race is real for `StructsToJson` (its evaluator does hold mutable
`writer`/`gen` and it accepts a non-foldable child), this override does **not**
fix it; the sharing would have to be broken where the evaluator actually lives
(inside `replacement`).
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/JsonExpressionsSuite.scala:
##########
@@ -1041,4 +1042,42 @@ class JsonExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
input)
}
+ test("JsonToStructs, GetJsonObject, JsonTuple are stateful and produce fresh
copies") {
+ val schema = StructType(StructField("a", IntegerType) :: Nil)
+ val jsonToStructs = JsonToStructs(schema, Map.empty, Literal("{}"),
UTC_OPT)
+ assert(jsonToStructs.stateful)
+ assert(jsonToStructs.freshCopyIfContainsStatefulExpression() ne
jsonToStructs)
+
+ val getJsonObject = GetJsonObject(Literal("{}"), Literal("$.a"))
+ assert(getJsonObject.stateful)
+ assert(getJsonObject.freshCopyIfContainsStatefulExpression() ne
getJsonObject)
+
+ val jsonTuple = JsonTuple(Literal("{}") :: Literal("a") :: Nil)
+ assert(jsonTuple.stateful)
+ assert(jsonTuple.freshCopyIfContainsStatefulExpression() ne jsonTuple)
+ }
+
+ test("StructsToJson and SchemaOfJson are stateful and produce fresh copies
with " +
Review Comment:
This test asserts `copy1.replacement...evaluator ne
copy2.replacement...evaluator` on the **pre-replacement** node — each
`freshCopy` trivially has its own `@transient lazy val evaluator`, so this
passes regardless. But it doesn't reflect the executed plan:
`ReplaceExpressions` materializes the `Invoke` once and the evaluator is shared
through the resulting `Literal` (see the `StructsToJson` comment). So for the
`RuntimeReplaceable` expressions this assertion is green without exercising the
runtime sharing it's meant to guard. If the intent is to prove `StructsToJson`
no longer races, the test needs to go through replacement (e.g. run
`ReplaceExpressions` / an end-to-end duplicated-projection query) rather than
inspecting `.replacement` on the un-replaced node.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/csvExpressions.scala:
##########
@@ -187,6 +188,8 @@ case class SchemaOfCsv(
override protected def withNewChildInternal(newChild: Expression):
SchemaOfCsv =
copy(child = newChild)
+ override def stateful: Boolean = true
Review Comment:
Same as `SchemaOfJson`: `SchemaOfCsv` is `RuntimeReplaceable`, requires a
foldable child, and `SchemaOfCsvEvaluator` holds only config lazy-vals —
constant-folded, so no race is possible and the override is unnecessary/dead at
execution. Inconsistent with the unmarked `SchemaOfXml`. Suggest dropping it.
--
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]
