cloud-fan commented on code in PR #58317:
URL: https://github.com/apache/spark/pull/58317#discussion_r3882881464
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcUtils.scala:
##########
@@ -437,6 +437,9 @@ object OrcUtils extends Logging {
s"array<${getOrcSchemaString(a.elementType)}>"
case m: MapType =>
s"map<${getOrcSchemaString(m.keyType)},${getOrcSchemaString(m.valueType)}>"
+ // Keep Spark responsible for CHAR/VARCHAR assignment and scan checks.
Native ORC
+ // CHAR/VARCHAR would truncate or pad before Spark can validate the
original value.
+ case _: CharType | _: VarcharType => StringType.catalogString
Review Comment:
This makes ORC return physical STRING values, but the V1 and V2 row paths
still construct `OrcDeserializer` with the logical `CharType`/`VarcharType`.
`OrcDeserializer.newWriter` matches only `case StringType`, so with standard
semantics enabled and `spark.sql.orc.enableVectorizedReader=false`, even an
in-range explicit-schema read fails as an unsupported type before padding or
length validation can run. Please teach the row decoder to handle all
`StringType` subtypes, including nested positions, and cover both reader modes
for V1 and V2.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1697,33 +1697,94 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
sql("DROP TEMPORARY FUNCTION IF EXISTS std_char_param")
sql("DROP TEMPORARY FUNCTION IF EXISTS std_varchar_param")
}
+ }
+ }
- // ORC catalog tables stamp the catalyst type so typeof survives
write/read.
- withTable("std_orc") {
- sql("CREATE TABLE std_orc (c CHAR(5), v VARCHAR(5)) USING orc")
- sql("INSERT INTO std_orc VALUES ('ab', 'cd')")
- assert(spark.table("std_orc").schema.map(_.dataType) ===
- Seq(CharType(5), VarcharType(5)))
- checkAnswer(
- sql("SELECT concat('<', c, '>'), concat('<', v, '>') FROM std_orc"),
- Row("<ab >", "<cd>"))
+ test("SPARK-58814: major formats preserve CHAR/VARCHAR schemas and values") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ Seq("parquet", "orc").foreach { format =>
+ Seq("v1" -> format, "v2" -> "").foreach { case (sourceVersion,
useV1List) =>
+ withSQLConf(SQLConf.USE_V1_SOURCE_LIST.key -> useV1List) {
+ withTempPath { dir =>
+ val path = dir.getCanonicalPath
+ val input = spark.range(1).selectExpr(
+ "cast('ab' AS CHAR(4)) AS c",
+ "cast('xy' AS VARCHAR(3)) AS v",
+ "named_struct('c', cast('z' AS CHAR(2))) AS s",
+ "array(cast('q' AS VARCHAR(2))) AS a",
+ "map(cast('k' AS CHAR(2)), cast('v' AS VARCHAR(2))) AS m")
+ input.write.mode("overwrite").format(format).save(path)
+
+ val readBack = spark.read.format(format).load(path)
+ assert(DataType.equalsIgnoreNullability(readBack.schema,
input.schema),
+ s"$format $sourceVersion lost CHAR/VARCHAR schema")
+ checkAnswer(
+ readBack.selectExpr("concat('<', c, '>')", "v", "concat('<',
s.c, '>')"),
Review Comment:
The input adds array and map CHAR/VARCHAR fields, but this action selects
only `c`, `v`, and `s.c`. Spark can prune `a` and `m`, so the test never
decodes those collection values or exercises their recursive scan checks.
Please select and assert `a` and `m` in both ORC source modes, and add
oversized nested struct, array-element, map-key, and map-value cases that must
raise `EXCEED_LIMIT_LENGTH`; otherwise native truncation in those branches
could regress while this test stays green.
--
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]
