srielau commented on code in PR #58130:
URL: https://github.com/apache/spark/pull/58130#discussion_r3836521302
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -4122,9 +4122,13 @@ case class Sentences(
*/
case class StringSplitSQL(
str: Expression,
- delimiter: Expression) extends BinaryExpression {
+ delimiter: Expression) extends BinaryExpression with
ImplicitCastInputTypes {
override def dataType: DataType =
- ArrayType(StringHelper.transformingStringResultType(str.dataType),
containsNull = false)
+ ArrayType(str.dataType, containsNull = false)
+ override def inputTypes: Seq[AbstractDataType] =
+ Seq(
+ StringTypeWithCollation(supportsTrimCollation = true),
+ StringTypeWithCollation(supportsTrimCollation = true))
Review Comment:
Same issue, smaller blast radius: `StringSplitSQL` is the `split_part`
implementation, and `SplitPart` already has `ImplicitCastInputTypes`. Mixing it
here is not needed for CHAR promotion.
Prefer `ExpectsInputTypes` so a directly constructed `StringSplitSQL` still
rejects non-string children instead of silently casting them.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -265,8 +265,12 @@ case class MultiGetJsonObject(
// scalastyle:on line.size.limit line.contains.tab
case class JsonTuple(children: Seq[Expression])
extends Generator
+ with ImplicitCastInputTypes
with QueryErrorsBase {
+ override def inputTypes: Seq[AbstractDataType] =
+ Seq.fill(children.size)(StringTypeWithCollation(supportsTrimCollation =
true))
Review Comment:
`ImplicitCastInputTypes` is more than CHAR/VARCHAR promotion. It also casts
INT and untyped NULL to STRING, so existing negatives stop failing:
- `json_tuple('{"a": 1}', 1)`
- `json_tuple('{"a": 1}', null)`
both currently `DATATYPE_MISMATCH.NON_STRING_TYPE` in `generators.sql` /
`table-valued-functions.sql`. This is not gated on `standardSemantics`.
`JsonTable` mixed in this trait so `JSON_TABLE(NULL, ...)` reaches runtime.
`JsonTuple` goldens encode the opposite contract.
Please mix `ExpectsInputTypes` only. The new CHAR/VARCHAR arm on
`ExpectsInputTypes` already inserts `CAST(... AS STRING)` without opening
general implicit casts. Then `fieldType` can follow the (promoted) JSON child.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionHelper.scala:
##########
@@ -651,14 +718,20 @@ abstract class TypeCoercionHelper {
case e: ExpectsInputTypes if e.inputTypes.nonEmpty =>
// Convert NullType into some specific target type for
ExpectsInputTypes that don't do
- // general implicit casting.
+ // general implicit casting. Also promote CHAR/VARCHAR to STRING here:
these
+ // expressions skip ImplicitCastInputTypes, so without this the length
constraint would
+ // remain on the child.
val children: Seq[Expression] = e.children.zip(e.inputTypes).map {
case (in, expected) =>
- if (in.dataType == NullType && !expected.acceptsType(NullType)) {
- Literal.create(null, expected.defaultConcreteType)
- } else {
- in
- }
+ charVarcharToPlainString(in.dataType, expected)
Review Comment:
This is the right hook for `ArrayJoin` / `StringToMap` (`ExpectsInputTypes`
only). JsonTuple and StringSplitSQL should use this path rather than
`ImplicitCastInputTypes`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionHelper.scala:
##########
@@ -133,6 +139,71 @@ abstract class TypeCoercionHelper {
*/
def implicitCast(e: Expression, expectedType: AbstractDataType):
Option[Expression]
+ /**
+ * Where a plain string is expected, promote CHAR(n)/VARCHAR(n) to unbounded
STRING the same way
+ * SHORT promotes to INT, and return the promoted type.
+ *
+ * CharType and VarcharType extend StringType, so an expectation such as
+ * `StringTypeWithCollation` accepts them as-is and the implicit cast rules
leave the length
+ * constraint in place. Expressions that then require all their string
inputs to share a single
+ * type (`overlay`, `string_agg`, ...) cannot unify CHAR(n) with STRING, and
RuntimeReplaceable
+ * ones (`right`) build literals from the constrained type that no longer
match their other
+ * branches.
+ *
+ * The expectation must actually mention a string type (or an array of
strings). Promoting at an
+ * `AnyDataType` site would strip the length from pass-through expressions
such as `max`, `lag`,
+ * and `element_at`, which are required to preserve CHAR/VARCHAR.
+ */
+ protected def charVarcharToPlainString(
+ inType: DataType,
+ expectedType: AbstractDataType): Option[DataType] = {
+ if (!conf.charVarcharStandardSemantics) {
+ return None
+ }
+ inType match {
+ case st: StringType if !StringHelper.isPlainString(st) =>
+ val plain = StringHelper.plainStringType(st)
+ if (expectsStringType(expectedType) &&
expectedType.acceptsType(plain)) {
+ Some(plain)
+ } else {
+ None
+ }
+ case ArrayType(et, containsNull) =>
+ arrayElementExpectation(expectedType).flatMap { elemExpected =>
+ charVarcharToPlainString(et, elemExpected).map(ArrayType(_,
containsNull))
+ }
+ case _ => None
+ }
+ }
+
+ private def expectsStringType(expectedType: AbstractDataType): Boolean =
expectedType match {
+ case _: StringType => true
+ case _: AbstractStringType => true
+ case TypeCollection(types) => types.exists(expectsStringType)
+ case _ => false
+ }
+
+ private def arrayElementExpectation(expectedType: AbstractDataType):
Option[AbstractDataType] =
+ expectedType match {
+ case AbstractArrayType(elem) => Some(elem)
+ case ArrayType(elem, _) => Some(elem)
+ case TypeCollection(types) =>
types.view.flatMap(arrayElementExpectation).headOption
+ case _ => None
+ }
+
+ /**
+ * Concat/Elt stringify non-binary inputs. Promote CHAR/VARCHAR to unbounded
STRING with the
+ * same collation instead of targeting the default UTF8_BINARY StringType,
which would not
+ * accept a collated constrained type.
+ */
+ protected def implicitCastToString(e: Expression): Expression = e.dataType
match {
+ case st: StringType
+ if conf.charVarcharStandardSemantics &&
!StringHelper.isPlainString(st) =>
+ implicitCast(e, StringHelper.plainStringType(st)).getOrElse(e)
+ case _ =>
+ implicitCast(e, StringType).getOrElse(e)
+ }
+
Review Comment:
This re-implements the flag check and `plainStringType` that
`charVarcharToPlainString` already owns. Concat/Elt still need a dedicated rule
(they are not `ImplicitCastInputTypes`), but the string case can go through the
shared helper, e.g. target `StringTypeWithCollation(supportsTrimCollation =
true)` or `charVarcharToPlainString(e.dataType, ...).map(Cast(e, _))`, and keep
`implicitCast(e, StringType)` only for non-strings.
Please also add Concat/Elt tests with `CHAR(n) COLLATE UTF8_LCASE` -- that
is the regression this branch exists to fix, and the new tests only cover
UTF8_BINARY.
--
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]
