srielau commented on code in PR #58130:
URL: https://github.com/apache/spark/pull/58130#discussion_r3836555042
##########
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 ExpectsInputTypes
Review Comment:
`ExpectsInputTypes` avoids INT -> STRING, but it still hits the NullType
rewrite in `ImplicitTypeCoercion` (`Literal.create(null,
expected.defaultConcreteType)`).
`json_tuple('{"a": 1}', null)` is `NON_STRING_TYPE` today (`generators.sql`,
`table-valued-functions.sql`). After this mix-in it becomes a typed STRING null
and analysis succeeds, including with `standardSemantics` off.
JsonTuple already has a custom `checkInputDataTypes`. Please promote
CHAR/VARCHAR without taking the NullType rewrite -- a dedicated arm that only
applies `charVarcharToPlainString` -- or regenerate those goldens if typed-null
is the intended contract.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionHelper.scala:
##########
@@ -651,14 +716,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)
+ .map(dt => if (dt == in.dataType) in else Cast(in, dt))
+ .getOrElse {
+ if (in.dataType == NullType &&
!expected.acceptsType(NullType)) {
+ Literal.create(null, expected.defaultConcreteType)
Review Comment:
This NullType branch is why mixing `ExpectsInputTypes` into JsonTuple is not
equivalent to CHAR-only promotion. JsonTuple needs the
`charVarcharToPlainString` arm without this rewrite.
--
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]
