srielau commented on code in PR #58132:
URL: https://github.com/apache/spark/pull/58132#discussion_r3817636520
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -72,14 +72,31 @@ object RowEncoder extends DataTypeErrorsBase {
encoderForDataType(schema, lenient).asInstanceOf[AgnosticEncoder[Row]]
}
+ /**
+ * Builds an encoder for a schema that the engine produced, such as the
result schema of a Spark
+ * Connect query. Whether CHAR/VARCHAR are first class types is decided by
the session that
+ * produced the schema, so they are always accepted here. A client cannot
read that session's
+ * configuration, and refusing the type would make the result undecodable.
+ */
+ private[sql] def encoderForResultSchema(schema: StructType):
AgnosticEncoder[Row] =
Review Comment:
Added `RowEncoderSuite` "SPARK-58794: encoderFor rejects CHAR/VARCHAR when
first-class types are off". With both `CHAR_VARCHAR_STANDARD_SEMANTICS` and
`PRESERVE_CHAR_VARCHAR_TYPE_INFO` off, `RowEncoder.encoderFor` still raises
`UNSUPPORTED_DATA_TYPE_FOR_ENCODER`; `encoderForResultSchema` accepts the same
schema so engine-produced Connect results remain decodable.
`7f6e4136ef5`.
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkResult.scala:
##########
@@ -101,7 +101,7 @@ private[sql] class SparkResult[T](
case UnboundRowEncoder =>
// Replace the row encoder with the encoder inferred from the schema.
RowEncoder
- .encoderFor(dataType.asInstanceOf[StructType])
+ .encoderForResultSchema(dataType.asInstanceOf[StructType])
Review Comment:
Kept `encoderForResultSchema` as the result-schema entry point.
`SparkResult.createEncoder` uses it only for `UnboundRowEncoder`; other encoder
bindings still go through `encoderFor`. The PR description now covers that
split, the Arrow STRING up-cast, and why JDBC CAST collect needed it.
Architecture of the new entry point is unchanged.
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/arrow/ArrowDeserializer.scala:
##########
@@ -135,8 +135,14 @@ object ArrowDeserializers {
new Deserializer[Any] {
def get(i: Int): Any = null
}
- case (StringEncoder, v: FieldVector) =>
- new LeafFieldDeserializer[String](encoder, v, timeZoneId) {
+ // CHAR/VARCHAR travel as plain Arrow string vectors; the length is part
of the type, not of
+ // the encoding, and the values arrive already padded and length checked
by the server. Read
+ // them against the unconstrained string type, since narrowing STRING to
CHAR(n)/VARCHAR(n)
+ // is not an up-cast and the reader would reject the vector.
+ case (StringEncoder | _: CharEncoder | _: VarcharEncoder, v:
FieldVector) =>
Review Comment:
Left the Arrow deserializer `plainStringType` comment as-is. Python Connect
already maps proto `char` / `var_char` in `pyspark.sql.connect.types`; added
`SparkConnectBasicTests.test_char_varchar_result_schema` (classic vs Connect
schema + `collect()` with standard semantics).
`CHAR_OCTET_LENGTH` stays character `n` (documented on
`JdbcTypeUtils.getCharOctetLength` and in the PR description). Connect and HS2
`getColumns` tests assert `=== n`; that is the CI gate rather than a semantic
change.
--
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]
