srielau commented on code in PR #59325:
URL: https://github.com/apache/spark/pull/59325#discussion_r4251013347
##########
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:
Update after rebasing onto master: this is now moot. The rebase picked up
SPARK-60090 (#59302), which removes the SPARK-59683 CHAR/VARCHAR codec from the
no-SerDe `TRANSFORM` path and instead reapplies CHAR/VARCHAR assignment
(including map keys) in a post-`TRANSFORM` `Project`. The
`charVarcharStandardSemantics` argument this comment is about no longer exists
in `BaseScriptTransformationExec`: the script reader keeps I/O as unbounded
`STRING` and rejects any residual CHAR/VARCHAR. This PR therefore drops its
`TRANSFORM` changes entirely and is now scoped to `from_json` and JSON file
scans, so the earlier fail-on-removal test discussion no longer applies.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -1873,7 +1873,14 @@ case class JsonToStructs(
options: Map[String, String],
child: Expression,
timeZoneId: Option[String] = None,
- variantAllowDuplicateKeys: Boolean =
SQLConf.get.getConf(SQLConf.VARIANT_ALLOW_DUPLICATE_KEYS))
+ variantAllowDuplicateKeys: Boolean =
SQLConf.get.getConf(SQLConf.VARIANT_ALLOW_DUPLICATE_KEYS),
+ // `spark.sql.charVarchar.standardSemantics.enabled` has PERSISTED
binding, so it is captured
+ // here at analysis time (as the default arg, like
`variantAllowDuplicateKeys`) and then
+ // threaded all the way into the parser via
`charVarcharStandardSemanticsOverride`. This keeps
+ // a view's CHAR/VARCHAR map-key semantics tied to its creation-time flag
rather than the
+ // caller's session setting. (`variantAllowDuplicateKeys` is only
captured, not threaded: the
+ // parser still reads that one live from `SQLConf.get`.)
+ charVarcharStandardSemantics: Boolean =
SQLConf.get.charVarcharStandardSemantics)
Review Comment:
Follow-up: after rebasing onto master this binding concern is gone.
SPARK-60090 (#59302) landed on master and removed the no-SerDe `TRANSFORM`
script-reader CHAR/VARCHAR codec, reapplying assignment (incl. map keys) in a
post-`TRANSFORM` `Project`. `BaseScriptTransformationExec` no longer constructs
`JsonToStructs` with a `charVarcharStandardSemantics` argument, so the
lazy-val-at-execution gap no longer exists in this PR.
`JsonToStructs.charVarcharStandardSemantics` is now only captured at analysis
for the `from_json` path, as before. The proposed persisted-view-over-TRANSFORM
test is therefore unnecessary.
--
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]
