srielau commented on code in PR #58080:
URL: https://github.com/apache/spark/pull/58080#discussion_r3817017413
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -184,7 +184,24 @@ object Project {
if (other == target) {
col
} else if (Cast.canANSIStoreAssign(other, target)) {
- Cast(col, target, Option(conf.sessionLocalTimeZone), ansiEnabled =
true)
+ // Store assignment must not use character-to-character CAST
truncation (ISO 6.13).
+ // Cast to unconstrained STRING first, then apply the write-side
length check.
+ // Avoid replaceCharVarcharWithString: first-class types keep
CHAR/VARCHAR.
+ val (castTarget, lengthCheckType) = target match {
+ case c: CharType => (c.toStringType, Some(c: DataType))
+ case v: VarcharType => (v.toStringType, Some(v: DataType))
+ case otherType => (otherType, None)
+ }
+ val casted = if (other == castTarget) {
+ col
+ } else {
+ Cast(col, castTarget, Option(conf.sessionLocalTimeZone),
ansiEnabled = true)
+ }
+ if (lengthCheckType.isDefined && !conf.charVarcharAsString) {
Review Comment:
This uses the raw legacy `charVarcharAsString` flag, but `standardSemantics`
takes precedence and keeps CHAR/VARCHAR first-class when both are true. In that
combination this skips `stringLengthCheck` and returns unconstrained STRING, so
`Dataset.to` can accept overflow.
Please apply the check whenever the actual target is CHAR/VARCHAR (ignore
the legacy flag if first-class types are present), and add the combined-config
regression test. `TableOutputResolver` has the same gate -- worth aligning if
INSERT can hit it too.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala:
##########
@@ -58,12 +58,16 @@ trait ToStringBase { self: UnaryExpression with
TimeZoneAwareExpression =>
// Returns a function to convert a value to pretty string. The function
assumes input is not null.
protected final def castToString(
from: DataType, to: StringConstraint = NoConstraint): Any => UTF8String =
- to match {
- case FixedLength(length) =>
+ (to, from) match {
+ case (FixedLength(length), _: StringType) if
SQLConf.get.charVarcharStandardSemantics =>
+ s => CharVarcharCodegenUtils.charTypeCast(castToString(from)(s),
length)
+ case (MaxLength(length), _: StringType) if
SQLConf.get.charVarcharStandardSemantics =>
Review Comment:
This is documented as explicit-CAST truncation, but `ToStringBase` is shared
by every `Cast` and has no explicit/implicit/store provenance. The Project
STRING-then-`stringLengthCheck` workaround is a reach-around to recover
assignment semantics.
Please distinguish explicit CAST vs store assignment (a Cast flag, or one
centralized store-assignment conversion all write paths use) so a new caller
cannot inherit truncation by accident.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala:
##########
@@ -115,6 +123,8 @@ object CollationTypeCoercion extends SQLConfHelper {
expr match {
case lit: Literal => lit.copy(dataType = newDataType)
+ case cast: Cast if needsCharPaddingCast(cast.dataType, newDataType)
=>
+ Cast(cast, newDataType, timeZoneId =
Some(conf.sessionLocalTimeZone))
case cast: Cast => cast.copy(dataType = newDataType)
Review Comment:
The CHAR-to-VARCHAR nest fixes the padding case (goldens now show the nested
cast and `false`). This arm still retargets every other Cast.
With truncating explicit CAST that changes semantics:
`COALESCE(CAST('abcdef' AS VARCHAR(2)), CAST('x' AS VARCHAR(4)))` can become
`CAST('abcdef' AS VARCHAR(4))` and return `abcd` instead of preserving `ab`.
Numeric / TRY_CAST overflow can likewise disappear.
Please preserve the explicit inner Cast and add an outer LCT Cast. Add
CAST/TRY_CAST character truncation and numeric overflow inside COALESCE/CASE/IN
(collated and not).
--
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]
