spark-test commented on code in PR #59812:
URL: https://github.com/apache/spark/pull/59812#discussion_r4384302058
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -393,7 +393,10 @@ private[sql] final case class LiteralValue[T](value: T,
dataType: DataType) exte
"NULL"
} else {
dataType match {
- case StringType => s"'${s"$value".replace("'", "''")}'"
+ // The StringType value pattern matches only the default UTF8_BINARY
collation.
Review Comment:
**Nit (P3):** This comment gives collation as the only reason `case
StringType =>` missed these literals. That stable-identifier pattern goes
through `StringType.equals`, which compares both `collationId` and `constraint`
(StringType.scala:89-94). So it also missed UTF8_BINARY `CHAR(n)`/`VARCHAR(n)`:
they carry a `FixedLength`/`MaxLength` constraint rather than `NoConstraint`
(CharType.scala:38-41, VarcharType.scala:37-40), and at the merge base they
also fell through to the unquoted `case _ => s"$value"` branch. The PR
description and the new `CharType(8)`/`VarcharType(8)` cases in
`LiteralValueSuite` cover this second reason, but someone reading only this
comment could narrow the match to a collation check and silently bring back
unquoted CHAR/VARCHAR literals. The V2PredicateSuite parity cases use only
collated `StringType`, so they would not catch that.
Possible wording, with the pattern-predicate sentence unchanged:
```scala
// The StringType value pattern matches only UTF8_BINARY strings
without a length
// constraint. Other collations and CHAR/VARCHAR are StringType
instances that need the
// same quoting and escaping.
// Pattern predicates also rely on these quotes when extracting the
literal contents.
```
**Verification:**
- **Inspection:** The revised comment accurately states that the replaced
value pattern excluded both non-default collations and length-constrained
CHAR/VARCHAR string types, stays ASCII, and keeps every line within the
100-character limit.
--
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]
