srielau commented on code in PR #58132:
URL: https://github.com/apache/spark/pull/58132#discussion_r3817597423
##########
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:
Correct call site: only `UnboundRowEncoder` (row `collect`) should ignore
local CHAR/VARCHAR config. Typed encoders stay as-is and still decode Arrow
string vectors via `StringEncoder`.
Worth a sentence in the PR description: without this, the JDBC CAST collect
assertions in this PR would fail on a client that only has `DefaultSqlApiConf`.
##########
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:
This is the right verb: engine-produced schema vs user-supplied schema,
instead of reading a session conf the client does not have.
Please add a negative test that `encoderFor` (not this method) still rejects
`CharType` / `VarcharType` when `charVarcharFirstClassTypes` is false, so UDF /
`createDataFrame` cannot pick up the result-schema override.
##########
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:
Reading CHAR/VARCHAR through `plainStringType` is the right explanation
(Arrow has no CHAR vector; narrowing is not an up-cast). Please keep the
comment; it is load-bearing.
The serializer change is needed for the ArrowEncoderSuite round trip.
Production `SparkResult` is decode-only, so skipping write-side length checks
here is acceptable -- just do not start using `encoderForResultSchema` to
encode user input.
--
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]
