cloud-fan commented on code in PR #58033:
URL: https://github.com/apache/spark/pull/58033#discussion_r3797675285
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -91,9 +91,9 @@ object RowEncoder extends DataTypeErrorsBase {
case DoubleType => BoxedDoubleEncoder
case dt: DecimalType => JavaDecimalEncoder(dt, lenientSerialization =
true)
case BinaryType => BinaryEncoder
- case c: CharType if SqlApiConf.get.preserveCharVarcharTypeInfo =>
+ case c: CharType if SqlApiConf.get.charVarcharFirstClassTypes =>
Review Comment:
**Blocking:**
Pass the full `CharType`/`VarcharType` into the leaf encoders instead of
only the length. `CharEncoder(length)` reconstructs a default-collation type,
and `EncoderField.structField` builds the resulting schema from that encoder
type, so `createDataFrame` loses an explicitly declared collation on this newly
enabled path. Please retain the original constrained type and add a collated
CHAR/VARCHAR encoder test.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -363,11 +363,12 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
plan.foreachUp {
case p if p.analyzed => // Skip already analyzed sub-plans
- case leaf: LeafNode if !SQLConf.get.preserveCharVarcharTypeInfo &&
- leaf.output.map(_.dataType).exists(CharVarcharUtils.hasCharVarchar) =>
+ case leaf: LeafNode if
leaf.output.map(_.dataType).exists(CharVarcharUtils.hasCharVarchar) &&
Review Comment:
**Non-blocking:**
Put `!SQLConf.get.charVarcharFirstClassTypes` first and use
`leaf.output.exists(attr => CharVarcharUtils.hasCharVarchar(attr.dataType))`.
In the newly enabled mode this guard can never match, but the current order
still maps and scans every leaf output before rejecting the case.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/util/CharVarcharCodegenUtils.java:
##########
@@ -64,4 +64,27 @@ public static UTF8String readSidePadding(UTF8String
inputStr, int limit) {
return inputStr;
}
}
+
+ /**
+ * Read-side CHAR check under standard semantics: pad to limit, or trim
trailing
+ * spaces then error if still longer than limit.
+ *
+ * Standard semantics require a read to observe the same value a write would
have
+ * produced, so this is deliberately the write-side check rather than
+ * {@link #readSidePadding}, which tolerates over-long values. Keep the two
sides
+ * identical: a fix to one is a fix to both.
+ */
+ public static UTF8String charTypeReadSideCheck(UTF8String inputStr, int
limit) {
+ return charTypeWriteSideCheck(inputStr, limit);
+ }
+
+ /**
+ * Read-side VARCHAR check under standard semantics: allow up to limit
characters,
+ * or trim trailing spaces then error if still longer than limit.
+ *
+ * Identical to the write-side check by design; see {@link
#charTypeReadSideCheck}.
Review Comment:
**Nit:**
This should link to `#varcharTypeWriteSideCheck`, which is the write-side
method this VARCHAR helper actually delegates to. The current CHAR read-side
link points readers to the wrong contract.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1040,6 +1210,41 @@ class FileSourceCharVarcharTestSuite extends
CharVarcharTestSuite with SharedSpa
}
}
+ test("SPARK-58801: standardSemantics scan pads CHAR and errors on oversize")
{
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ withTempPath { dir =>
+ withTable("t") {
+ sql("SELECT '12' as col").write.format(format).save(dir.toString)
+ sql(s"CREATE TABLE t (col CHAR(3)) using $format LOCATION '$dir'")
+ checkAnswer(sql("SELECT * FROM t"), Row("12 "))
+ }
+ }
+ Seq("CHAR", "VARCHAR").foreach { typ =>
+ withTempPath { dir =>
+ withTable("t") {
+ sql("SELECT '123456' as
col").write.format(format).save(dir.toString)
+ sql(s"CREATE TABLE t (col $typ(2)) using $format LOCATION '$dir'")
+ checkError(
+ exception = intercept[SparkRuntimeException] {
+ sql("SELECT * FROM t").collect()
+ },
+ condition = "EXCEED_LIMIT_LENGTH",
+ parameters = Map("limit" -> "2")
+ )
+ }
+ }
+ }
+ // Oversize that is only trailing blanks trims successfully.
Review Comment:
**Nit:**
Please rephrase this as `An oversized value consisting only of trailing
blanks is trimmed successfully.` The current sentence uses `oversize` as a noun
and is hard to parse.
--
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]
