cloud-fan commented on code in PR #58581:
URL: https://github.com/apache/spark/pull/58581#discussion_r3992292084
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/SparkSession.scala:
##########
@@ -223,7 +223,10 @@ class SparkSession private[sql] (
/** @inheritdoc */
def createDataFrame(rows: java.util.List[Row], schema: StructType):
DataFrame = {
- createDataset(RowEncoder.encoderFor(schema),
rows.iterator().asScala).toDF()
+ // RowEncoder consults the client process's local SqlApiConf, which can
differ from the
+ // server-side configuration visible through SparkSession.conf. Encode an
explicitly provided
+ // schema independently of that local configuration, as for result schemas.
+ createDataset(RowEncoder.encoderForResultSchema(schema),
rows.iterator().asScala).toDF()
Review Comment:
**Blocking (P1):** This path still bypasses
`spark.sql.legacy.charVarcharAsString=true`. `encoderForResultSchema` pads CHAR
values before the server policy is known, and `buildLocalRelationFromRows`
later restores the original CHAR/VARCHAR attributes even when `Dataset.to`
normalized the target to STRING. For example, `CharType(4)` with `"ab"` can
come back as CHAR with `"ab "` instead of STRING with `"ab"`. Could we keep
the physical row values unpadded until server-side reconciliation and cover
legacy, standard, and default policies for both empty and populated input?
**Recommended change:** Separate the physical row encoder/schema from the
requested logical schema for caller-provided Row data. Serialize CHAR/VARCHAR
leaves as collation-preserving physical STRING values without length checks,
carry the original schema separately, and have buildLocalRelationFromRows
construct its final attributes from the server-policy-normalized target rather
than unconditionally from the original schema.
**Why this works:** Keeping raw strings on the wire prevents irreversible
client-side CHAR padding. Server-side Dataset.to reconciliation can then apply
default rejection, standard-semantics preservation, or legacy-as-string
normalization once, and the resulting relation schema can match that selected
policy.
**Scope:** sql/connect/common, sql/connect/server, sql/connect/client/jvm
**Compatibility:** Result-schema decoding may continue to use first-class
CHAR/VARCHAR encoders; the separation applies to caller-provided local data
whose legality is owned by the server session.
**Risks:** Changing the shared local-relation builder could affect
non-CHAR/VARCHAR caller schemas or chunked serialization. The physical STRING
type must retain the requested collation so Arrow conversion does not alter
string interpretation before reconciliation.
**Constraints:** Do not use the client's local SqlApiConf to decide the
server's CHAR/VARCHAR policy. Preserve standard-semantics padding and length
checks, default-policy rejection, collation, nullability, and nested schema
behavior. Keep empty and populated local relations on the same server
reconciliation path.
**Success:** With legacy charVarcharAsString enabled and standard semantics
disabled, explicit CHAR/VARCHAR input yields STRING schema and unpadded values.
With standard semantics enabled, explicit CHAR/VARCHAR input retains its
logical schema and existing padding and length behavior. With neither mode
enabled, empty and populated explicit CHAR/VARCHAR input fail with the same
unsupported-type error. Nested CHAR/VARCHAR leaves and collations follow the
same policy matrix.
##########
python/pyspark/sql/types.py:
##########
@@ -330,19 +330,31 @@ class CharType(AtomicType):
----------
length : int
the length limitation.
+ collation : str, optional
Review Comment:
**Nit (P3):** Could this parameter documentation state that the default is
`None`, meaning no explicitly declared collation, and distinguish it from
explicit `UTF8_BINARY`? The latter is semantically binary but emits restoration
metadata, while omission does not. The same clarification is needed for
`VarcharType`.
##########
python/pyspark/sql/types.py:
##########
@@ -2671,7 +2708,12 @@ def _parse_datatype_json_value( # type: ignore[return]
def _assert_valid_type_for_collation(
fieldPath: str, fieldType: Any, collationMap: Dict[str, str]
) -> None:
- if fieldPath in collationMap and fieldType != "string":
+ is_string_type = (
+ fieldType == "string"
+ or (isinstance(fieldType, str) and _LENGTH_CHAR.fullmatch(fieldType)
is not None)
Review Comment:
**Blocking (P1):** This validation also accepts an already-collated
constrained type when `__COLLATIONS` supplies the same field. For example,
inline `char(4) collate UTF8_LCASE` plus metadata `spark.UTF8_BINARY` silently
returns `CharType(4, 'UTF8_BINARY')`, discarding the inline declaration; the
JVM rejects the dual encoding. Could we require uncollated
`char(n)`/`varchar(n)` text whenever restoration metadata is present and add
matching/conflicting negative cases?
##########
python/pyspark/sql/types.py:
##########
@@ -330,19 +330,31 @@ class CharType(AtomicType):
----------
length : int
the length limitation.
+ collation : str, optional
+ name of the collation.
"""
- def __init__(self, length: int):
+ def __init__(self, length: int, collation: Optional[str] = None):
self.length = length
+ self.collation = collation
def simpleString(self) -> str:
- return "char(%d)" % (self.length)
+ if self.collation is None:
+ return "char(%d)" % (self.length)
+
+ return "char(%d) collate %s" % (self.length, self.collation)
def jsonValue(self) -> str:
- return "char(%d)" % (self.length)
+ return self.simpleString()
def __repr__(self) -> str:
Review Comment:
**Nit (P3):** Please add collated `CharType` and `VarcharType` instances to
the existing `eval(repr(instance)) == instance` coverage, including explicit
`UTF8_BINARY` and a non-binary collation. The current implementation
round-trips, but neither new conditional repr branch has a durable regression
signal.
--
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]
