[SPARK-58794][SQL] Promote CHAR/VARCHAR to STRING via ImplicitTypeCasts

[srielau]

What changes were proposed in this pull request?

Under spark.sql.charVarchar.standardSemantics.enabled, an expression that expects a plain string now gets CHAR(n) / VARCHAR(n) promoted to unbounded STRING by the analyzer, the same way SHORT is promoted to INT. Previously each transforming expression stripped the length constraint in its own dataType.

• Move charVarcharToPlainString onto TypeCoercionHelper and recurse into arrays when the expected type is AbstractArrayType / TypeCollection (for example, array_join).
• Apply the promotion in ImplicitTypeCasts for both ImplicitCastInputTypes and ExpectsInputTypes.
• Use collation-preserving implicitCastToString for Concat / Elt instead of implicitCast(e, StringType), which missed collated CHAR.
• Drop the per-expression StringHelper.transformingStringResultType stripping; a result type now follows its promoted child.
• Use ExpectsInputTypes for StringSplitSQL.
• Give JsonTuple a CHAR/VARCHAR-only coercion arm so its existing INT and untyped NULL validation remains unchanged.

Why are the changes needed?

Stripping the constraint inside each expression missed every ExpectsInputTypes site (str_to_map, array_join, and others) and collated CHAR on Concat, and it had to be repeated in each new expression. Promoting at the call site is the same mechanism Spark already uses for numeric widening, so it applies uniformly and shows up in the analyzed plan as a CAST(... AS STRING).

Does this PR introduce any user-facing change?

Yes, when the flag is on: analyzed plans for transforming string operators now contain an explicit CAST(... AS STRING) and those operators return unbounded STRING. Result values are unchanged versus the previous per-expression stripping.

How was this patch tested?

• TypeCoercionSuite: legacy and ANSI Concat / Elt promotion, including CHAR(n) COLLATE UTF8_LCASE; ImplicitTypeCasts promotion; and unchanged JsonTuple INT / untyped NULL handling.
• BasicCharVarcharTestSuite: transforming string result types and collated Concat / Elt values.
• Regenerated charvarchar-standard-semantics.sql result and analyzer goldens.

Was this patch authored or co-authored using generative AI tooling?

Yes (Cursor).

[srielau]

Rebased onto #58087 (6dc3909). Analyzer goldens updated for concat promotion after the LCT CHAR-pad plan change.

[srielau]

SQL Language review (7871f0f)

Promoting CHAR/VARCHAR to unbounded STRING in implicitCast, before acceptsType, is the right place. CHAR subclasses StringType, so the old per-expression transformingStringResultType was papering over a type-coercion hole. Gating on charVarcharStandardSemantics, skipping AnyDataType, and leaving LCT / explicit CAST / store assignment alone looks correct. Array recursion for array_join is justified. Concat/Elt still need their own rule because they bypass generic ImplicitTypeCasts. Dual-run coverage on shared TypeCoercionHelper is the right resolver story.

Findings

1. High -- JsonTuple now does general implicit casts, unflagged. Mixing ImplicitCastInputTypes is not CHAR promotion. json_tuple('{"a": 1}', 1) and json_tuple('{"a": 1}', null) currently fail DATATYPE_MISMATCH.NON_STRING_TYPE (generators.sql, table-valued-functions.sql). After this change they succeed (INT/NULL -> STRING), even with standardSemantics off. JsonTable mixed in the same trait on purpose so untyped NULL reaches runtime; JsonTuple goldens encode the opposite contract.

   Use ExpectsInputTypes (not ImplicitCastInputTypes) and keep inputTypes. The new ExpectsInputTypes arm already promotes CHAR/VARCHAR without opening numeric/NULL casts.

2. Medium -- implicitCastToString duplicates the new promotion. Flag check + plainStringType already live in charVarcharToPlainString. Concat/Elt can call that (or implicitCast(e, StringTypeWithCollation(supportsTrimCollation = true))) and fall back to implicitCast(e, StringType) for non-strings.

3. Medium -- collated Concat/Elt is the stated reason for implicitCastToString and is untested. New Concat/Upper tests use UTF8_BINARY CHAR only. Please add CHAR(...) COLLATE UTF8_LCASE for Concat and Elt (and ANSI coercion).

4. Low -- StringSplitSQL does not need ImplicitCastInputTypes.split_part already has it. Prefer ExpectsInputTypes here too, same as JsonTuple.

Architecture of the central promotion is sound. Finding 1 is the blocker: it changes SQL outside the flag and contradicts existing goldens this PR does not regenerate.

[srielau]

SQL Language re-review (f0675dd)

Previous items:

1. JsonTuple / StringSplitSQL: switched to ExpectsInputTypes. INT no longer implicit-casts. Good.
2. implicitCastToString: now delegates to charVarcharToPlainString. Good.
3. Collated Concat/Elt: TypeCoercionSuite (ANSI + non-ANSI), CharVarcharTestSuite, and goldens cover UTF8_LCASE. Good.

Remaining

1. High -- json_tuple(..., null) is still a contract change. Mixing ExpectsInputTypes opts JsonTuple into the existing NullType rewrite in ImplicitTypeCoercion:

   Literal.create(null, expected.defaultConcreteType)

   Untyped null becomes a STRING null beforecheckInputDataTypes. select json_tuple('{"a": 1}', null) currently fails NON_STRING_TYPE in generators.sql / table-valued-functions.sql. INT is fixed; NULL is not. This is also unflagged (standardSemantics off).

   Options: (a) do not mix ExpectsInputTypes and promote CHAR only in a JsonTuple-specific arm that skips the NullType rewrite; (b) accept the NULL change and regenerate those goldens. (a) matches the previous review.

2. Low -- PR description is stale. It still says mix ImplicitCastInputTypes into JsonTuple and StringSplitSQL.

The extra CHAR(2) -> CHAR(3) in the new collated concat analyzer golden comes from CollationTypeCoercion on Concat children (LCT), not from this promotion. Not blocking here.

[srielau]

SQL Language re-review (4132d65)

The JsonTuple NULL issue is fixed. JsonTuple no longer mixes ExpectsInputTypes / ImplicitCastInputTypes. CHAR/VARCHAR promotion is a dedicated ImplicitTypeCoercion arm that only applies charVarcharToPlainString, so untyped NULL and INT stay for checkInputDataTypes (NON_STRING_TYPE). Rule tests pin all three: NULL unchanged, INT unchanged, CHAR -> CAST(... AS STRING) under the flag. ANSI/non-ANSI share TypeCoercionHelper, so both paths get the arm.

Previous mediums (shared implicitCastToString, collated Concat/Elt coverage) remain in good shape.

Remaining (non-blocking)

1. Low -- PR description is still stale. It still says mix ImplicitCastInputTypes into JsonTuple and StringSplitSQL. The actual contract is: dedicated JsonTuple arm; StringSplitSQL uses ExpectsInputTypes.

2. Low -- JsonTuple rule tests sit in cast NullType for expressions that implement ExpectsInputTypes. JsonTuple is no longer that. A one-line comment on the case j: JsonTuple arm (custom checkInputDataTypes; do not take the NullType rewrite) would also help the next editor.

No new architectural issues. Not blocking on the lows.

[srielau]

Thanks for the re-review. Both lows are addressed in ff948fd.

1. PR description. This one was already fixed -- the description was updated at the same minute the review was posted, so you were reading the previous snapshot. It currently reads:

• Use ExpectsInputTypes for StringSplitSQL.
• Give JsonTuple a CHAR/VARCHAR-only coercion arm so its existing INT and untyped NULL validation remains unchanged.

2. JsonTuple tests and the arm comment. The rule tests moved out of cast NullType for expressions that implement ExpectsInputTypes into their own coerce JsonTuple children without the NullType rewrite, which is what they actually pin: NULL unchanged, INT unchanged, CHAR promoted under the flag. The Upper CHAR case stays behind in the original test, since that one really is an ExpectsInputTypes expression.

The arm itself now carries the warning:

// JsonTuple validates its own input types and rejects non-string children with// NON_STRING_TYPE, so it only takes the CHAR/VARCHAR promotion here. Do not fold this into// the ExpectsInputTypes arm below: that would also apply the NullType rewrite and turn// json_tuple(json, null) from an analysis error into a typed STRING null.

TypeCoercionSuite and AnsiTypeCoercionSuite pass (108 tests).

[cloud-fan]

0 blocking, 2 non-blocking, 0 nits.
The coercion design is sound, with two non-blocking comment and test-integrity issues to address.

Correctness (2)

• Non-blocking: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:2710: Update the Right comment: both analyzers inspect unresolved RuntimeReplaceable replacements, so COMPLEX_EXPRESSION_UNSUPPORTED_INPUT is not the mismatch failure path. -- see inline
• Non-blocking: sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:1848: Make the CHAR/VARCHAR analyzer-parity test assert that each query actually enters dual-run; ResolverGuard rejection currently falls back to fixed-point silently. -- see inline

Verification

I traced the promotion from the legacy, ANSI, and single-pass analyzer entry points into the shared TypeCoercionHelper, including the configuration gate, collation preservation, array recursion, and the dedicated JsonTuple path that leaves INT and untyped NULL for its validator. I also checked the changed rule/value/golden expectations against those paths. I did not run tests during this review.

[cloud-fan]

0 addressed, 2 remaining, 0 new to this AI review.
0 blocking, 2 non-blocking, 0 nits.
The coercion design is sound; two non-blocking comment and test-integrity issues remain.

Remaining from prior review (2)

• Non-blocking: Remove the inaccurate analyzer-failure claim from Right; both analyzers reject an unresolved replacement during analysis, so COMPLEX_EXPRESSION_UNSUPPORTED_INPUT is not the later failure path. -- existing thread
• Non-blocking: Make the analyzer-parity matrix assert that each query actually enters dual-run; ResolverGuard can otherwise route it silently to fixed-point analysis. -- existing thread

Verification

I traced scalar and array promotion through legacy, ANSI, and single-pass coercion; confirmed the flag, collation, AnyDataType, and JsonTuple gates; and checked the changed expression metadata and test expectations against those paths. I also verified both analyzers reject unresolved RuntimeReplaceable replacements and that HybridAnalyzer can bypass dual-run when ResolverGuard returns an unsupported reason. I did not run tests during this review.

[srielau]

Thanks @cloud-fan -- addressed the two remaining items from #58130 (review) in de3f84f.

• Dropped the inaccurate COMPLEX_EXPRESSION_UNSUPPORTED_INPUT failure-path claim on Right. Both analyzers reject an unresolved RuntimeReplaceable.replacement during analysis, so that is not a later runtime path. Thread: [SPARK-58794][SQL] Promote CHAR/VARCHAR to STRING via ImplicitTypeCasts #58130 (comment)
• The SPARK-58802 matrix now fails if ResolverGuard would skip dual-run. Each SELECT is parsed and checked with a fresh ResolverGuard before execution; CREATE/INSERT setup uses spark.sql directly. Thread: [SPARK-58794][SQL] Promote CHAR/VARCHAR to STRING via ImplicitTypeCasts #58130 (comment)

The targeted single-pass resolver agrees with fixed-point under standardSemantics test passes.

[cloud-fan]

2 addressed, 0 remaining, 0 new to this AI review.
0 blocking, 0 non-blocking, 0 nits.
The current revision is ready from a code-review perspective; the previously reported concerns are addressed and no actionable correctness, design, performance, or test-coverage issues remain.

Verification

Reviewed the complete diff and traced the changed coercion helpers through their legacy, ANSI, fixed-point, and single-pass call paths. Checked constrained scalar and array promotion, TypeCollection fallback behavior, collation preservation, specialized Concat/Elt handling, JsonTuple error preservation, result-type derivation, and the updated unit, integration, and SQL golden test coverage. Tests were not run locally.

[cloud-fan]

Merge Summary:

• merged into master 85eb4c8
• merged into branch-4.x 65efa0c

Posted by merge_spark_pr.py