Re: [PR] [SPARK-58814][SQL] Cover CHAR/VARCHAR round-trips and stop ORC truncating scans [spark]

["cloud-fan (via GitHub)" <gi...@apache.org>]

cloud-fan commented on code in PR #58317:
URL: https://github.com/apache/spark/pull/58317#discussion_r4186287967


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcUtils.scala:
##########
@@ -536,11 +558,14 @@ object OrcUtils extends Logging {
       resultSchema: StructType,
       partitionSchema: StructType,
       conf: Configuration): String = {
-    val resultSchemaString = if (canPruneCols) {
-      OrcUtils.getOrcSchemaString(resultSchema)
+    val readSchema = if (canPruneCols) {
+      resultSchema
     } else {
-      OrcUtils.getOrcSchemaString(StructType(dataSchema.fields ++ partitionSchema.fields))
+      StructType(dataSchema.fields ++ partitionSchema.fields)
     }
+    // Spark CHAR/VARCHAR in the read schema are Catalyst constraints; ask ORC for STRING.
+    val resultSchemaString =
+      OrcUtils.getOrcSchemaString(orcReadSchemaWithoutCharVarchar(readSchema))

Review Comment:
   **Non-blocking (P2):** This rewrite runs whenever first-class CHAR/VARCHAR types are enabled. That includes the released `spark.sql.preserveCharVarcharTypeInfo` (4.0.0), not only standard semantics. Before this change ORC got `varchar(n)`/`char(n)` here, and its string-group conversion truncated STRING-stored values to `n`. In preserve-only mode Spark adds no read-side VARCHAR check (`addPaddingForScan` only pads CHAR there), so nothing takes over from ORC.
   
   Concretely: with `preserveCharVarcharTypeInfo=true` and standard semantics off, `spark.read.schema("v VARCHAR(4)").orc(p)` over a stored `abcdef` used to return `abcd` on the default vectorized reader. It now returns `abcdef` in a `VARCHAR(4)` column, with no error. CHAR(4) behaves the same way.
   
   Keeping the physical read independent of session flags looks intentional, and it matches Parquet. However, the PR description says the user-facing change is limited to unreleased standard semantics. The new preserve-only result is not declared in the `preserveCharVarcharTypeInfo` doc or a migration note, and no test pins it: the preserve-only assertions added here only compare schemas.
   
   **Recommended change:** Keep the mode-independent STRING reader and make the preserve-only outcome explicit. (1) Extend the PRESERVE_CHAR_VARCHAR_TYPE_INFO doc to say that ORC reads with CHAR/VARCHAR schemas over STRING storage return stored values without ORC truncation (as Parquet does), and that read-side length checks apply only under spark.sql.charVarchar.standardSemantics.enabled. (2) Add a SQL migration guide entry for the preserve-only ORC behavior change. (3) Add preserve-only ORC value tests in the CHAR/VARCHAR test family. (4) The PR description's user-facing-change section should mention the preserve-only change.
   
   **Why this works:** The physical ORC read stays independent of session flags, which preserves the single interpretation that fixed the cross-mode cache identity problems. The config contract and migration guide declare the new preserve-only outcome, and value-level tests make it a pinned, intentional behavior rather than an incidental one.
   
   **Scope:** Documentation of the released preserve-only config and migration guide, plus preserve-only ORC value tests; no production code change.
   
   **Compatibility:** The mode-independent ORC reader schema and standard-semantics read-side checks.
   
   **Risks:** Users of the experimental preserve flag who relied on ORC truncation will see longer values; the migration note must say so and point to standard semantics for read-side checks.
   
   **Constraints:** Do not reintroduce execution-time SQLConf reads into the ORC reader schema. Keep standard-semantics EXCEED_LIMIT_LENGTH and padding behavior unchanged.
   
   **Success:** The PRESERVE_CHAR_VARCHAR_TYPE_INFO doc and the migration guide state that preserve-only ORC reads over STRING storage are not truncated and that read-side length checks require standard semantics. A preserve-only ORC read of a stored abcdef value with a VARCHAR(4) schema returns abcdef in vectorized and row readers for V1 and V2, and the test fails if ORC truncation returns. Standard-semantics ORC reads of the same data still raise EXCEED_LIMIT_LENGTH.
   
   **Verification:**
   - **Behavior:** In preserve-only mode, ORC reads with over-length VARCHAR and CHAR schemas over STRING-stored data return the stored values (CHAR padded when shorter) across vectorized and row readers and V1/V2 sources.
   - **Compatibility:** Under standard semantics the same reads raise EXCEED_LIMIT_LENGTH, and default-mode ORC reads are unchanged.
   - **Inspection:** The preserveCharVarcharTypeInfo config documentation and the migration guide describe the preserve-only ORC read outcome and how it relates to standard semantics.



##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveCharVarcharTestSuite.scala:
##########
@@ -54,12 +55,22 @@ class HiveCharVarcharTestSuite extends CharVarcharTestSuite with TestHiveSinglet
     }
   }
 
-  test("SPARK-35700: Read char/varchar orc table with created and written by external systems") {
+  test("SPARK-35700, SPARK-58814: read native char/varchar ORC") {
     withTable("t") {
       hiveClient.runSqlHive("CREATE TABLE t(c CHAR(5), v VARCHAR(7)) STORED AS ORC")
-      hiveClient.runSqlHive("INSERT INTO t VALUES('Spark', 'kyuubi')")
-      checkAnswer(sql("SELECT c, v from t"), Row("Spark", "kyuubi"))
-      checkAnswer(sql("SELECT v from t where c = 'Spark' and v = 'kyuubi'"), Row("kyuubi"))
+      hiveClient.runSqlHive("INSERT INTO t VALUES('Sp', 'kyuubi')")
+      Seq(true, false).foreach { vectorizedReaderEnabled =>
+        withSQLConf(
+            SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true",

Review Comment:
   **Nit (P3):** Every assertion now runs under standard semantics, so the default-configuration case that SPARK-35700 fixed is no longer tested. That case is a pushed `v = 'kyuubi'` predicate over a Hive-written native ORC `varchar(7)` column, read through `convertMetastoreOrc`. Under standard semantics both `c` and `v` are wrapped in read-side checks, so neither predicate reaches `OrcFilters`. No other Hive test pushes a predicate over native ORC CHAR/VARCHAR in default mode.
   
   Because this PR edits `OrcFilters.getPredicateLeafType` and the ORC schema code, a default-mode pushdown regression would now pass CI. Could you keep a default-configuration iteration, with the original `'Spark'`/`'kyuubi'` data and predicate, next to the new standard-semantics one?
   
   **Verification:**
   - **Regression:** A default-configuration read of a Hive-written native ORC CHAR(5)/VARCHAR(7) table with a pushed VARCHAR equality predicate returns the expected row, and fails if default-mode pushdown or schema stripping regresses.



##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1976,17 +2138,6 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
           assert(spark.read.orc(path).schema.head.dataType === StringType)
         }
       }
-      // preserveCharVarcharTypeInfo also keeps first-class types, so write still stamps CHAR.

Review Comment:
   **Nit (P3):** This was the only test that wrote ORC with `preserveCharVarcharTypeInfo` on and standard semantics off, then checked that a file-only read infers `CHAR(4)`. The preserve-only round trip added earlier for the review request on `OrcUtils` (write first-class CHAR/VARCHAR, reload without a schema) is gone too.
   
   Every ORC write in the changed tests now runs under standard semantics. The preserve-only reads only compare the schemas of files written in standard mode, and the generic `CharVarcharTestSuite` variants run on parquet and DSV2, not ORC. So if preserve-only writes stopped stamping `spark.sql.catalyst.type`, no test would fail.
   
   Could you add back a preserve-only ORC write followed by a file-only read that asserts the inferred `CHAR(4)`/`VARCHAR(4)` types?
   
   **Verification:**
   - **Regression:** A preserve-only ORC write of CHAR(4)/VARCHAR(4) values reloads with constrained types inferred, and fails if preserve-only writes stop stamping spark.sql.catalyst.type.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org