srielau commented on code in PR #59325:
URL: https://github.com/apache/spark/pull/59325#discussion_r4248917853
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/BaseScriptTransformationExec.scala:
##########
@@ -307,7 +310,11 @@ trait BaseScriptTransformationExec extends UnaryExecNode {
}
case _: ArrayType | _: MapType | _: StructType =>
val complexTypeFactory = JsonToStructs(attr.dataType,
- ioschema.outputSerdeProps.toMap, Literal(null),
Some(conf.sessionLocalTimeZone))
+ ioschema.outputSerdeProps.toMap, Literal(null),
Some(conf.sessionLocalTimeZone),
+ // Bind the parse-time flag so a CHAR/VARCHAR map key reaching this
fallback (standard
+ // semantics off, preserveCharVarcharTypeInfo on) keeps the
TRANSFORM-time behavior
+ // rather than picking up the executing session's flag at this
lazy-val execution.
+ charVarcharStandardSemantics = standardCharVarcharSemantics)
Review Comment:
You're right that the binding is the only thing keeping a CHAR/VARCHAR-keyed
map output on the parse-time mode, and I agree it should be guarded. I tried to
add exactly the test you describe (a `MAP<CHAR(3), INT>` output beside the
SPARK-59683 conf-change case: planned with standard semantics off and
`preserveCharVarcharTypeInfo` on, executed with it on, script output
`{"a":1}`), but I could not make it fail when the fallback binding is removed,
and I don't want to add a test that passes either way.
The reason is the harness, not the fix. `BaseScriptTransformationSuite`
drives these through `QueryTest.executePlan`, which runs
`execution.executedPlan.executeCollectPublic()` with no `SQLExecution`
execution id, so the SQL confs are not propagated to the task.
`outputFieldWriters` is forced on the "Executor task launch worker" thread, and
there `SQLConf.get` reads the default conf, not the `withSQLConf` override on
the driver thread. I instrumented the fallback branch to confirm it: executing
under `withSQLConf(standardSemantics = true)` with the plan bound to `false`,
it prints `bound=false live=false` on the executor thread. So in every
reachable case here the live value equals the bound value, and dropping the
binding is unobservable. The scalar SPARK-59683 case works precisely because it
keys off the serialized `ioschema.standardCharVarcharSemantics` field rather
than a live `SQLConf.get`.
The binding is still correct and needed for the real execution path (where
the task does read the propagated/execution-time conf), so I've kept it,
consistent with the scalar guarantee.
The one way I see to get genuine parse-vs-execution conf divergence through
a conf-propagating path is a persisted view over a no-SerDe TRANSFORM with a
`MAP<CHAR(n), _>` output (the flag has PERSISTED binding), created flag-off and
queried flag-on via a normal `collect()`. It's a fair bit more machinery and
has a few TRANSFORM-in-view unknowns. Happy to pursue that as the regression
test if you'd like it in this PR, or to leave the binding guarded by review and
track the end-to-end test as a follow-up. Which do you prefer?
--
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]
