srielau commented on code in PR #58033:
URL: https://github.com/apache/spark/pull/58033#discussion_r3800859874
##########
sql/core/src/test/resources/sql-tests/inputs/charvarchar-standard-semantics.sql:
##########
@@ -0,0 +1,90 @@
+--SET spark.sql.charVarchar.standardSemantics.enabled=true
+
+-- R3: CAST introduces CHAR/VARCHAR
+SELECT typeof(CAST('ab' AS CHAR(5)));
+SELECT typeof(CAST('hello' AS VARCHAR(5)));
+SELECT 'X' || CAST('5' AS CHAR(5)) || 'X';
+
+-- CAST length enforcement: trailing spaces are trimmed, real overflow errors
+SELECT CAST('ab ' AS CHAR(2));
+SELECT CAST('abcdef' AS CHAR(2));
+SELECT CAST('abcdef' AS VARCHAR(2));
+SELECT try_cast('abcdef' AS CHAR(2));
+SELECT try_cast('abcdef' AS VARCHAR(2));
+
+-- R2: least common type (COALESCE / CASE)
+SELECT typeof(coalesce(cast('hello' AS VARCHAR(5)), cast('world' AS
VARCHAR(10))));
+SELECT typeof(coalesce(cast('hello' AS VARCHAR(5)), cast('world!' AS
CHAR(6))));
+SELECT typeof(coalesce(cast('hello' AS CHAR(5)), cast('world!' AS CHAR(6))));
+SELECT typeof(coalesce(cast('hello' AS VARCHAR(5)), 'world'));
+SELECT typeof(coalesce(cast('hello' AS CHAR(5)), NULL));
+SELECT typeof(
+ CASE WHEN true THEN cast('a' AS CHAR(2)) ELSE cast('bb' AS CHAR(4)) END);
+
+-- R2: least common type for IN lists
+SELECT cast('a' AS CHAR(2)) IN (cast('a ' AS CHAR(2)), cast('bbb' AS
VARCHAR(3)));
+SELECT typeof(c) FROM (SELECT cast('a' AS CHAR(2)) AS c) t WHERE c IN ('a ',
'b');
+
+-- R1: transforming functions return STRING
+SELECT typeof(upper(cast('ab' AS CHAR(2))));
+SELECT typeof(lower(cast('AB' AS VARCHAR(2))));
+SELECT typeof(cast('a' AS CHAR(1)) || cast('b' AS VARCHAR(1)));
+SELECT typeof(substr(cast('hello' AS VARCHAR(5)), 1, 2));
+SELECT typeof(upper(coalesce(cast('a' AS CHAR(2)), cast('b' AS CHAR(4)))));
+SELECT typeof(concat(cast('a' AS CHAR(2)), cast('b' AS CHAR(3))));
+SELECT typeof(trim(cast('ab ' AS CHAR(4))));
+SELECT typeof(lpad(cast('ab' AS CHAR(2)), 5, 'x'));
+
+-- R1: regexp / mask / split family
+SELECT typeof(regexp_replace(cast('ab' AS CHAR(2)), 'a', 'x'));
+SELECT typeof(regexp_extract(cast('ab' AS VARCHAR(2)), '(a)', 1));
+SELECT typeof(regexp_extract_all(cast('aab' AS VARCHAR(3)), '(a)', 1));
+SELECT typeof(split(cast('a,b' AS CHAR(3)), ','));
+SELECT typeof(mask(cast('ab' AS CHAR(2))));
+
+-- R1: CHAR/VARCHAR promote to STRING where a plain string is expected, so
expressions that
+-- require all their string inputs to share one type accept them alongside a
STRING argument.
+-- Values are wrapped in sentinels because the golden format trims trailing
blanks, which would
+-- otherwise hide the CHAR padding these expressions operate on.
+SELECT typeof(overlay(cast('ab' AS CHAR(5)) PLACING 'x' FROM 1));
+SELECT concat('<', overlay(cast('ab' AS CHAR(5)) PLACING 'x' FROM 1), '>');
+SELECT typeof(elt(1, cast('ab' AS CHAR(5)), 'x'));
+SELECT typeof(right(cast('ab' AS CHAR(5)), 2));
+SELECT concat('<', right(cast('ab' AS CHAR(5)), 2), '>');
+SELECT typeof(left(cast('ab' AS CHAR(5)), 2));
+
+-- R1: transforms whose result length differs from the input must not inherit
the constraint.
+SELECT typeof(reverse(cast('ab' AS CHAR(5))));
+SELECT typeof(hex(cast('ab' AS CHAR(5))));
+SELECT hex(cast('ab' AS CHAR(5)));
+SELECT typeof(array_join(array(cast('ab' AS CHAR(5)), cast('cd' AS CHAR(5))),
'-'));
+SELECT concat('<', array_join(array(cast('ab' AS CHAR(5)), cast('cd' AS
CHAR(5))), '-'), '>');
+-- reverse() on a non-string input is unaffected.
+SELECT typeof(reverse(array(1, 2)));
+
+-- R2 with collation.
+-- The recorded "string collate null" is a pre-existing gap, not a
standardSemantics behavior:
Review Comment:
**High (Golden/Tests):** Please don’t golden `string collate null` for
collated CHAR coalesce. The LCT helper unit test already expects `CharType(4,
UTF8_LCASE)`, so this SQL/`typeof` result is a real E2E gap in the foundation —
not just documentation of a known quirk.
Prefer removing/ignoring the query until fixed, or regenerating against the
correct type. Otherwise we normalize a bug and a later fix looks like a golden
regression.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/StringType.scala:
##########
@@ -159,25 +159,76 @@ case object StringHelper extends
PartialOrdering[StringConstraint] {
def isPlainString(s: StringType): Boolean = s.constraint == NoConstraint
+ /**
+ * Strip CHAR/VARCHAR length constraints, preserving collation.
+ *
+ * Used by transforming string expressions (upper, substr, concat, ...) so
their result type is
+ * plain STRING even when inputs are CharType/VarcharType (SQL standard
CHAR/VARCHAR R1), when
+ * standard semantics are on.
+ */
+ def plainStringType(dt: DataType): DataType = dt match {
+ case c: CharType => c.toStringType
+ case v: VarcharType => v.toStringType
+ case other => other
+ }
+
+ def plainStringType(s: StringType): StringType = s match {
+ case c: CharType => c.toStringType
+ case v: VarcharType => v.toStringType
+ case other => other
+ }
+
+ /**
+ * Result type for transforming string expressions. Under
+ * spark.sql.charVarchar.standardSemantics.enabled, always plain STRING
(R1). Under
+ * preserveCharVarcharTypeInfo alone, keep child type (legacy leaky path).
+ */
+ def transformingStringResultType(dt: DataType): DataType = {
Review Comment:
**High (Architecture):** R1 is currently a dual protocol
(`charVarcharToPlainString` + per-expr `transformingStringResultType`). That
will keep regressing — anything using `ExpectsInputTypes` (no cast) or
returning `child`/`first.dataType` without the helper still leaks
`CharType`/`VarcharType` (see `StringToMap` in the review summary).
Can we centralize “transforming string result → plain STRING under
standardSemantics” (trait / shared base) and keep an explicit R2/R3 allowlist
for pass-throughs (`coalesce`, `max`, `element_at`, …)? At minimum, please add
an inventory test that fails when a string-producing expr still returns
constrained types under the flag.
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/RowSetUtils.scala:
##########
@@ -147,8 +147,12 @@ object RowSetUtils {
// types that reach this branch do not use the `nested` flag in
`toHiveString`. Now,
// Geospatial types use it for wrapping EWKT in quotes when nested
= true, so we need
// to set `nested` here to false to avoid spurious quotes for
standalone geo values.
+ // String types need the same treatment: the fast path above
matches only the
+ // default-collation StringType singleton, so CHAR/VARCHAR and
collated strings land
+ // here and would otherwise be rendered as "value" instead of
value.
val nested = typ match {
case _: GeometryType | _: GeographyType => false
+ case _: StringType => false
Review Comment:
**Medium (Compatibility):** The `nested = false` for `_: StringType` also
fixes collated strings on the Thrift fallback path, ungated by
`standardSemantics`. That’s probably right, but it’s a broader behavior change
than the CHAR/VARCHAR flag.
Please document it explicitly in the PR description and add a test (collated
string + CHAR/VARCHAR) so we don’t regress quoting again. Consider splitting to
a tiny dedicated bugfix if a cleaner changelog is preferred.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -453,7 +454,8 @@ trait String2StringExpression extends
ImplicitCastInputTypes {
def convert(v: UTF8String): UTF8String
- override def dataType: DataType = child.dataType
+ override def dataType: DataType =
+ StringHelper.transformingStringResultType(child.dataType)
Review Comment:
**Medium (Architecture):** Putting R1 on `String2StringExpression` also
changes `Empty2Null`, which isn’t a transforming function (empty→null is
pass-through for non-empty values). V1Writes currently only applies it when
`dataType == StringType`, so we’re safe by accident.
Please override `Empty2Null.dataType` to keep the child type (or don’t share
that trait).
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -1430,7 +1430,10 @@ case class Reverse(child: Expression)
BinaryType,
ArrayType))
- override def dataType: DataType = child.dataType
+ // Reversing a string transforms its content, so a CHAR/VARCHAR input yields
plain STRING (R1).
+ // Array and binary inputs are unaffected. The promotion in
ImplicitTypeCasts does not reach
+ // here because the expected type is a TypeCollection rather than a plain
string type.
Review Comment:
**Low (Style):** Nit: `expectsStringType` already treats `TypeCollection`,
so ImplicitTypeCasts *does* promote CHAR for `Reverse`’s string branch. Mind
updating the comment so we don’t teach the wrong invariant?
(`transformingStringResultType` remains useful as belt-and-suspenders / for
non-cast paths.)
--
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]
