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: [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]
