srielau commented on code in PR #58080:
URL: https://github.com/apache/spark/pull/58080#discussion_r3815769592
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala:
##########
@@ -429,8 +432,18 @@ object CollationTypeCoercion extends SQLConfHelper {
(left.strength.priority, right.strength.priority) match {
case (leftPriority, rightPriority) if leftPriority == rightPriority =>
- if (left.sameType(right)) left
- else handleMismatch()
+ if (left.sameType(right)) {
+ left
+ } else {
+ // Equal strength with differing types is only a real collation
mismatch when the
+ // collations differ. Same-collation CHAR(2) and CHAR(4) differ only
in length, so widen
+ // to the string-family LCT (max(n, m), which pads rather than
truncates); a genuine
+ // collation mismatch has no LCT and falls through.
Review Comment:
This is load-bearing method behavior rather than a tactical inline note.
Could we move it into `getWinningStringType`'s scaladoc with a concrete SQL /
type transformation example, then keep the branch itself self-explanatory?
Also worth stating in that scaladoc that `CollationTypeCoercion` is not
gated on `standardSemantics` alone (`charVarcharFirstClassTypes` is true under
`preserveCharVarcharTypeInfo` as well).
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala:
##########
@@ -429,8 +432,18 @@ object CollationTypeCoercion extends SQLConfHelper {
(left.strength.priority, right.strength.priority) match {
case (leftPriority, rightPriority) if leftPriority == rightPriority =>
- if (left.sameType(right)) left
- else handleMismatch()
+ if (left.sameType(right)) {
+ left
+ } else {
+ // Equal strength with differing types is only a real collation
mismatch when the
+ // collations differ. Same-collation CHAR(2) and CHAR(4) differ only
in length, so widen
+ // to the string-family LCT (max(n, m), which pads rather than
truncates); a genuine
+ // collation mismatch has no LCT and falls through.
+ StringHelper.tightestCommonString(left.stringType, right.stringType)
match {
+ case Some(lct) => StringTypeWithContext(lct, left.strength)
+ case None => handleMismatch()
+ }
+ }
case (leftPriority, rightPriority) =>
if (leftPriority < rightPriority) left
Review Comment:
This still couples collation precedence to the CHAR/VARCHAR length. The LCT
branch only runs when priorities are equal; with an explicit `CHAR(2) COLLATE
UTF8_LCASE` and an implicit same-collation `CHAR(4)`, this branch returns the
entire `CHAR(2)` context and narrows the other operand.
Can we resolve the winning collation strength separately from the
string-family LCT? Same-collation inputs should use `max(n, m)` regardless of
strength, with the stronger strength attached afterward. Please add an
explicit-vs-implicit mixed-length golden case (this is a narrowing /
runtime-error risk, not just a typeof nit).
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1201,6 +1201,108 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
}
}
+ test("SPARK-58794: compare, IN and set-ops cast every participant to the
string LCT") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ // Comparison and IN cast both sides (including the IN left-hand side)
to the LCT of all
+ // participants. Casting to CHAR pads, so unequal CHAR lengths compare
equal after widen;
+ // casting to VARCHAR/STRING keeps the CHAR pad, so equality needs a
matching blank.
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = cast('a' AS CHAR(4))"),
Row(true))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = cast('a' AS
VARCHAR(2))"), Row(false))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = cast('a ' AS
VARCHAR(2))"), Row(true))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = 'a'"), Row(false))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = 'a '"), Row(true))
+
+ // INTypeCoercion uses findWiderCommonType over (lhs +: list), then
casts every child:
+ // the left-hand side is not exempt.
+ val inAnalyzed = sql(
+ "SELECT cast('a' AS CHAR(2)) IN (cast('a' AS CHAR(4)), cast('b' AS
VARCHAR(3)))")
+ .queryExecution.analyzed
+ assert(inAnalyzed.toString.contains("as varchar(4)"),
+ s"LHS and list should all widen to VARCHAR(4), got:\n$inAnalyzed")
+ checkAnswer(
+ sql("SELECT cast('a' AS CHAR(2)) IN (cast('a' AS CHAR(4)))"),
Row(true))
+ checkAnswer(
+ sql("SELECT cast('a' AS CHAR(2)) IN (cast('a' AS VARCHAR(2)))"),
Row(false))
+ checkAnswer(
+ sql("SELECT cast('a' AS CHAR(2)) IN (cast('a ' AS VARCHAR(2)))"),
Row(true))
+
+ // RTRIM ignores trailing blanks for CHAR vs STRING and for mixed CHAR
lengths.
+ checkAnswer(
+ sql("SELECT cast('a' AS CHAR(2) COLLATE UTF8_BINARY_RTRIM) = 'a'"),
Row(true))
+ checkAnswer(
+ sql("""SELECT cast('a' AS CHAR(2) COLLATE UTF8_BINARY_RTRIM) =
+ | cast('a' AS CHAR(4) COLLATE
UTF8_BINARY_RTRIM)""".stripMargin),
+ Row(true))
+
+ // Collated LCT must keep the collation and take max(n, m), not become
indeterminate.
+ assert(sql(
+ """SELECT coalesce(
+ | cast('a' AS CHAR(2) COLLATE UTF8_LCASE),
+ | cast('bb' AS CHAR(4) COLLATE UTF8_LCASE)) AS c""".stripMargin)
+ .schema.head.dataType === CharType(4, "UTF8_LCASE"))
+
+ // Set ops and multi-row VALUES share the same LCT.
+ assert(sql(
+ """SELECT c FROM (
+ | SELECT cast('a' AS CHAR(2)) AS c UNION SELECT cast('a' AS
CHAR(4)) AS c) t"""
+ .stripMargin)
+ .schema.head.dataType === CharType(4))
+ checkAnswer(
+ sql("""SELECT c FROM (
+ | SELECT cast('a' AS CHAR(2)) AS c UNION SELECT cast('a' AS
CHAR(4)) AS c) t"""
+ .stripMargin),
+ Row("a "))
+ checkAnswer(
+ sql("""SELECT c FROM (
+ | SELECT cast('ab' AS CHAR(2)) AS c INTERSECT
+ | SELECT cast('ab' AS CHAR(4)) AS c) t""".stripMargin),
+ Row("ab "))
+ checkAnswer(
+ sql("""SELECT c FROM (
+ | SELECT cast('ab' AS CHAR(2)) AS c EXCEPT
+ | SELECT cast('ab' AS CHAR(4)) AS c) t""".stripMargin),
+ Seq.empty)
+ assert(sql(
+ """SELECT c FROM (VALUES (cast('a' AS CHAR(2))), (cast('bb' AS
CHAR(4)))) t(c)""")
+ .schema.head.dataType === CharType(4))
+ }
+ }
+
+ test("SPARK-58794: parameterized CHAR/VARCHAR lengths under
standardSemantics") {
Review Comment:
This parameter-marker test is independent of the collation LCT fix. Please
remove it from this PR, or move any genuinely missing CAST/error coverage to a
dedicated parameter suite.
The negative cases should also assert structured errors with `checkError`
rather than a bare `intercept`.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1201,6 +1201,108 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
}
}
+ test("SPARK-58794: compare, IN and set-ops cast every participant to the
string LCT") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ // Comparison and IN cast both sides (including the IN left-hand side)
to the LCT of all
+ // participants. Casting to CHAR pads, so unequal CHAR lengths compare
equal after widen;
+ // casting to VARCHAR/STRING keeps the CHAR pad, so equality needs a
matching blank.
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = cast('a' AS CHAR(4))"),
Row(true))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = cast('a' AS
VARCHAR(2))"), Row(false))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = cast('a ' AS
VARCHAR(2))"), Row(true))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = 'a'"), Row(false))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = 'a '"), Row(true))
+
+ // INTypeCoercion uses findWiderCommonType over (lhs +: list), then
casts every child:
+ // the left-hand side is not exempt.
+ val inAnalyzed = sql(
+ "SELECT cast('a' AS CHAR(2)) IN (cast('a' AS CHAR(4)), cast('b' AS
VARCHAR(3)))")
+ .queryExecution.analyzed
+ assert(inAnalyzed.toString.contains("as varchar(4)"),
+ s"LHS and list should all widen to VARCHAR(4), got:\n$inAnalyzed")
+ checkAnswer(
+ sql("SELECT cast('a' AS CHAR(2)) IN (cast('a' AS CHAR(4)))"),
Row(true))
+ checkAnswer(
+ sql("SELECT cast('a' AS CHAR(2)) IN (cast('a' AS VARCHAR(2)))"),
Row(false))
+ checkAnswer(
+ sql("SELECT cast('a' AS CHAR(2)) IN (cast('a ' AS VARCHAR(2)))"),
Row(true))
+
+ // RTRIM ignores trailing blanks for CHAR vs STRING and for mixed CHAR
lengths.
+ checkAnswer(
+ sql("SELECT cast('a' AS CHAR(2) COLLATE UTF8_BINARY_RTRIM) = 'a'"),
Row(true))
+ checkAnswer(
+ sql("""SELECT cast('a' AS CHAR(2) COLLATE UTF8_BINARY_RTRIM) =
+ | cast('a' AS CHAR(4) COLLATE
UTF8_BINARY_RTRIM)""".stripMargin),
+ Row(true))
+
+ // Collated LCT must keep the collation and take max(n, m), not become
indeterminate.
+ assert(sql(
+ """SELECT coalesce(
+ | cast('a' AS CHAR(2) COLLATE UTF8_LCASE),
+ | cast('bb' AS CHAR(4) COLLATE UTF8_LCASE)) AS c""".stripMargin)
+ .schema.head.dataType === CharType(4, "UTF8_LCASE"))
+
+ // Set ops and multi-row VALUES share the same LCT.
+ assert(sql(
+ """SELECT c FROM (
+ | SELECT cast('a' AS CHAR(2)) AS c UNION SELECT cast('a' AS
CHAR(4)) AS c) t"""
+ .stripMargin)
+ .schema.head.dataType === CharType(4))
+ checkAnswer(
+ sql("""SELECT c FROM (
+ | SELECT cast('a' AS CHAR(2)) AS c UNION SELECT cast('a' AS
CHAR(4)) AS c) t"""
+ .stripMargin),
+ Row("a "))
+ checkAnswer(
+ sql("""SELECT c FROM (
+ | SELECT cast('ab' AS CHAR(2)) AS c INTERSECT
+ | SELECT cast('ab' AS CHAR(4)) AS c) t""".stripMargin),
+ Row("ab "))
+ checkAnswer(
+ sql("""SELECT c FROM (
+ | SELECT cast('ab' AS CHAR(2)) AS c EXCEPT
+ | SELECT cast('ab' AS CHAR(4)) AS c) t""".stripMargin),
+ Seq.empty)
+ assert(sql(
+ """SELECT c FROM (VALUES (cast('a' AS CHAR(2))), (cast('bb' AS
CHAR(4)))) t(c)""")
+ .schema.head.dataType === CharType(4))
+ }
+ }
+
+ test("SPARK-58794: parameterized CHAR/VARCHAR lengths under
standardSemantics") {
+ // Length positions accept parameter markers (`integerValue` ->
`parameterMarker`). Under
+ // standardSemantics the bound type stays first-class: CAST keeps
CHAR/VARCHAR, pads and
+ // enforces length, and DDL schemas retain the substituted n.
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ val charDf = spark.sql("SELECT cast('ab' AS CHAR(:n)) AS c", Map("n" ->
5))
+ assert(charDf.schema.head.dataType === CharType(5))
+ checkAnswer(
+ spark.sql("SELECT concat('<', cast('ab' AS CHAR(:n)), '>')", Map("n"
-> 5)),
+ Row("<ab >"))
+
+ val varcharDf = spark.sql("SELECT cast('hello' AS VARCHAR(?)) AS c",
Array(5))
+ assert(varcharDf.schema.head.dataType === VarcharType(5))
+ intercept[SparkRuntimeException] {
+ spark.sql("SELECT cast('abcdef' AS VARCHAR(?))", Array(2)).collect()
+ }
+
+ withTable("param_varchar", "param_char") {
+ spark.sql(
+ "CREATE TABLE param_varchar (c VARCHAR(:n)) USING parquet", Map("n"
-> 7))
+ assert(spark.table("param_varchar").schema.head.dataType ===
VarcharType(7))
+ spark.sql("CREATE TABLE param_char (c CHAR(?)) USING parquet",
Array(4))
+ assert(spark.table("param_char").schema.head.dataType === CharType(4))
+ }
+
+ // Non-integral / negative lengths fail when substituted into the length
position.
+ intercept[ParseException] {
+ spark.sql("SELECT cast('a' AS CHAR(:n))", Map("n" -> -1)).collect()
+ }
+ intercept[ParseException] {
+ spark.sql("SELECT cast('a' AS CHAR(:n))", Map("n" -> 1.5)).collect()
+ }
+ }
+ }
+
test("SPARK-58802: single-pass resolver agrees with fixed-point under
standardSemantics") {
Review Comment:
The new tests run outside the SPARK-58802 dual-run block, so the actual
collated mixed-length regression is not checked against Analyzer++. Expression
coercion shares `CollationTypeCoercion`, but set operations have a separate
resolver path.
Please add the fixed COALESCE / compare / IN cases and mixed-CHAR `UNION
ALL` to this dual-run matrix.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1201,6 +1201,108 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
}
}
+ test("SPARK-58794: compare, IN and set-ops cast every participant to the
string LCT") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ // Comparison and IN cast both sides (including the IN left-hand side)
to the LCT of all
+ // participants. Casting to CHAR pads, so unequal CHAR lengths compare
equal after widen;
+ // casting to VARCHAR/STRING keeps the CHAR pad, so equality needs a
matching blank.
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = cast('a' AS CHAR(4))"),
Row(true))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = cast('a' AS
VARCHAR(2))"), Row(false))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = cast('a ' AS
VARCHAR(2))"), Row(true))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = 'a'"), Row(false))
+ checkAnswer(sql("SELECT cast('a' AS CHAR(2)) = 'a '"), Row(true))
+
+ // INTypeCoercion uses findWiderCommonType over (lhs +: list), then
casts every child:
+ // the left-hand side is not exempt.
+ val inAnalyzed = sql(
+ "SELECT cast('a' AS CHAR(2)) IN (cast('a' AS CHAR(4)), cast('b' AS
VARCHAR(3)))")
+ .queryExecution.analyzed
+ assert(inAnalyzed.toString.contains("as varchar(4)"),
Review Comment:
These queries substantially duplicate `charvarchar-standard-semantics.sql`.
Also, `inAnalyzed.toString.contains("as varchar(4)")` does not prove that the
IN LHS was widened; it can match a list-element cast.
Please add this exact three-part IN query to the analyzer golden file and
inspect the complete plan, then drop the duplicated unit assertions.
##########
sql/core/src/test/resources/sql-tests/inputs/charvarchar-standard-semantics.sql:
##########
@@ -66,22 +66,74 @@ SELECT typeof(reverse(array(1, 2)));
SELECT typeof(str_to_map(cast('a:1,b:2' AS CHAR(7))));
SELECT typeof(c0) FROM (SELECT json_tuple(cast('{"a":"1"}' AS CHAR(9)), 'a')
AS c0);
--- R2 with collation. A declared collation survives the CAST and an LCT over
equally constrained
--- operands. The mixed-length case (CHAR(2) with CHAR(4), same collation) is
deliberately not
--- covered here: CollationTypeCoercion reads the differing lengths as a
collation mismatch and
--- yields an indeterminate collation. That predates this change (it reproduces
under
--- spark.sql.preserveCharVarcharTypeInfo) and is tracked separately, so
goldening it would
--- normalize the bug.
+-- Collation survives CAST and LCT. Mixed lengths with the same collation
widen to max(n, m);
+-- they must not collapse to an indeterminate collation.
SELECT typeof(cast('a' AS CHAR(2) COLLATE UTF8_LCASE));
SELECT typeof(coalesce(
cast('a' AS CHAR(2) COLLATE UTF8_LCASE), cast('bb' AS CHAR(2) COLLATE
UTF8_LCASE)));
+SELECT typeof(coalesce(
+ cast('a' AS CHAR(2) COLLATE UTF8_LCASE), cast('bb' AS CHAR(4) COLLATE
UTF8_LCASE)));
+SELECT concat('<', coalesce(
+ cast('a' AS CHAR(2) COLLATE UTF8_LCASE), cast('bb' AS CHAR(4) COLLATE
UTF8_LCASE)), '>');
--- UNION LCT
+-- Set operations and multi-row VALUES share the same LCT as COALESCE.
SELECT typeof(c) FROM (
SELECT cast('a' AS VARCHAR(3)) AS c
UNION ALL
Review Comment:
The coverage matrix does not currently substantiate the PR description:
`UNION ALL` is only VARCHAR(3)/VARCHAR(8), the new set-op cases are uncollated
(so they do not hit the changed `CollationTypeCoercion` equal-strength branch),
and there is no collated mixed CHAR/VARCHAR case. EXCEPT's empty result also
cannot show padding.
Please add mixed-CHAR `UNION ALL`, collated CHAR/VARCHAR LCT/compare/IN, and
a non-empty EXCEPT result that visibly checks padding.
--
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]
