cloud-fan commented on code in PR #58581:
URL: https://github.com/apache/spark/pull/58581#discussion_r4068528416
##########
sql/api/src/main/scala/org/apache/spark/sql/types/CharType.scala:
##########
@@ -23,6 +23,10 @@ import org.apache.spark.sql.catalyst.util.CollationFactory
/**
* A data type representing fixed-length character strings with a specified
length.
*
+ * A standalone collated `CharType` writes its collation inline in JSON and
therefore requires a
+ * current reader. Within a [[StructField]], schema JSON stores the collation
in field metadata
+ * and emits an uncollated `char(n)` type so preceding readers can still read
the schema.
Review Comment:
**Nit (P3):** `preceding readers` sounds like reader or parser order, but
this compatibility boundary is about Spark versions. Could the parallel
CharType and VarcharType notes say `older readers` or `readers from earlier
Spark versions` so users know which readers preserve the collation?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/DataType.scala:
##########
@@ -469,24 +539,49 @@ object DataType {
/**
* Returns a map of field path to collation name.
*/
- private def getCollationsMap(metadataFields: List[JField]): Map[String,
String] = {
- val collationsJsonOpt = metadataFields.find(_._1 ==
COLLATIONS_METADATA_KEY).map(_._2)
- collationsJsonOpt match {
+ private def getCollationsMap(
+ metadataFields: List[JField],
+ metadataKey: String): Map[String, String] = {
+ val requireCompleteMap = metadataKey ==
CHAR_VARCHAR_COLLATIONS_METADATA_KEY
+ metadataFields.find(_._1 == metadataKey).map(_._2) match {
case Some(JObject(fields)) =>
- fields.collect { case (fieldPath, JString(collation)) =>
- collation.split("\\.", 2) match {
- case Array(provider: String, collationName: String) =>
- CollationFactory.assertValidProvider(provider)
- fieldPath -> collationName
- }
- }.toMap
-
+ val parsed = Map.newBuilder[String, String]
Review Comment:
**Non-blocking (P2):** The parser validates each restoration value but never
verifies that every path was used. For a field `c: char(4)`, metadata
containing `"typo": "spark.UTF8_LCASE"` matches no traversal point, yet the
reader removes the reserved key and returns an uncollated `CharType(4)`. Please
reject any leftover path before stripping the metadata in both Scala and Python
so a typo cannot silently change the schema.
See **Shared repair plan 1** in the review body.
##########
python/pyspark/sql/types.py:
##########
@@ -2674,6 +2765,41 @@ def _parse_datatype_json_value( # type: ignore[return]
)
+def _parse_collation_metadata_map(metadata: Optional[Dict[str, Any]], key:
str) -> Dict[str, str]:
+ if not metadata or key not in metadata:
+ return {}
+
+ raw = metadata[key]
+ if key == _CHAR_VARCHAR_COLLATIONS_METADATA_KEY:
+ if not isinstance(raw, dict):
+ raise _invalid_char_varchar_collation_metadata(raw)
+ parsed: Dict[str, str] = {}
+ for path, value in raw.items():
+ name_parts = value.split(".") if isinstance(value, str) else None
+ if name_parts is None or len(name_parts) != 2:
Review Comment:
**Non-blocking (P2):** `spark.` passes the two-element split check here, so
Python constructs `CharType(4, '')` and reserializes it as `icu.`; Scala
rejects the same schema. Please require both the provider and collation-name
components to be non-empty before accepting the restoration entry.
See **Shared repair plan 1** in the review body.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/DataType.scala:
##########
@@ -425,12 +468,16 @@ object DataType {
("name", JString(name)),
("nullable", JBool(nullable)),
("type", dataType: JValue)) =>
- val collationsMap = getCollationsMap(metadataFields)
- val metadataWithoutCollations =
- JObject(metadataFields.filterNot(_._1 == COLLATIONS_METADATA_KEY))
+ val collationsMap = getCollationsMap(metadataFields,
COLLATIONS_METADATA_KEY)
+ val charVarcharCollationsMap =
+ getCollationsMap(metadataFields, CHAR_VARCHAR_COLLATIONS_METADATA_KEY)
Review Comment:
Confirmed on the current head: both Scala UDT object branches validate the
outer __CHAR_VARCHAR_COLLATIONS target before returning, without traversing
UDT.sqlType.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4040154030","thread_id":"inline:4040154030","verdict_sha256":"b7c8c20b59a27a62b38100b999aea0571fb95ab54bd57ec94b5fa8d7aafe6730"}
-->
##########
python/pyspark/sql/types.py:
##########
@@ -2429,8 +2493,8 @@ def fromWKB(cls, wkb: bytes, srid: int) -> "Geometry":
"interval": CalendarIntervalType,
}
-_LENGTH_CHAR = re.compile(r"char\(\s*(\d+)\s*\)")
-_LENGTH_VARCHAR = re.compile(r"varchar\(\s*(\d+)\s*\)")
+_LENGTH_CHAR = re.compile(r"char\(\s*(\d+)\s*\)(?:\s+collate\s+(\w+))?")
Review Comment:
Confirmed on the current head: standalone Python CHAR/VARCHAR JSON parsing
uses full matches and rejects trailing tokens.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4040154037","thread_id":"inline:4040154037","verdict_sha256":"b7c8c20b59a27a62b38100b999aea0571fb95ab54bd57ec94b5fa8d7aafe6730"}
-->
##########
python/pyspark/sql/types.py:
##########
@@ -324,47 +324,82 @@ def isUTF8BinaryCollation(self) -> bool:
class CharType(AtomicType):
- """Char data type
+ """Char data type.
+
+ A standalone collated ``CharType`` writes its collation inline in JSON and
therefore requires
Review Comment:
Confirmed on the current head: the CharType/VarcharType docs now explain
that older Python readers may accept only the constrained-type prefix and lose
the inline collation.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4040154040","thread_id":"inline:4040154040","verdict_sha256":"b7c8c20b59a27a62b38100b999aea0571fb95ab54bd57ec94b5fa8d7aafe6730"}
-->
--
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]
