srielau commented on code in PR #58033:
URL: https://github.com/apache/spark/pull/58033#discussion_r3801134189
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/RowSetUtils.scala:
##########
@@ -147,8 +147,12 @@ object RowSetUtils {
// types that reach this branch do not use the `nested` flag in
`toHiveString`. Now,
// Geospatial types use it for wrapping EWKT in quotes when nested
= true, so we need
// to set `nested` here to false to avoid spurious quotes for
standalone geo values.
+ // String types need the same treatment: the fast path above
matches only the
+ // default-collation StringType singleton, so CHAR/VARCHAR and
collated strings land
+ // here and would otherwise be rendered as "value" instead of
value.
val nested = typ match {
case _: GeometryType | _: GeographyType => false
+ case _: StringType => false
Review Comment:
Correct that it is broader than the flag, and deliberately so: only the
default-collation `StringType` singleton matches the fast path above, so
collated strings have always fallen into the generic branch and been rendered
with quotes. Gating the fix on `standardSemantics` would leave that bug in
place for collated strings, which have nothing to do with CHAR/VARCHAR.
Added `RowSetUtilsSuite`, which calls `toTRowSet` directly and asserts
unquoted rendering for `CharType`, `VarcharType`, a `UTF8_LCASE` string and
plain `StringType`. I verified it is actually load-bearing: with the `case _:
StringType => false` line removed it fails with `List("\"ab\"") did not equal
List("ab")`. I put it at that level rather than in the JDBC suites because it
pins exactly the changed branch and runs in a second; the CHAR/VARCHAR
end-to-end path stays covered by the golden file through
`ThriftServerQueryTestSuite`, which is how the quoting bug surfaced in the
first place.
I will call the collated-string fix out in the PR description. Happy to
split it into a standalone bugfix if you prefer the cleaner changelog -- it is
self-contained now that the test is separate.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -453,7 +454,8 @@ trait String2StringExpression extends
ImplicitCastInputTypes {
def convert(v: UTF8String): UTF8String
- override def dataType: DataType = child.dataType
+ override def dataType: DataType =
+ StringHelper.transformingStringResultType(child.dataType)
Review Comment:
Agreed -- "safe by accident" is exactly right, and relying on the `dataType
== StringType` guard in `V1Writes` to keep it safe is not something I want to
depend on. `Empty2Null` now overrides `dataType` to return the child's type,
with a comment saying why it does not take the trait's R1 result. Kept it on
the trait rather than moving it off, since it is a string-to-string function in
every other respect.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -1430,7 +1430,10 @@ case class Reverse(child: Expression)
BinaryType,
ArrayType))
- override def dataType: DataType = child.dataType
+ // Reversing a string transforms its content, so a CHAR/VARCHAR input yields
plain STRING (R1).
+ // Array and binary inputs are unaffected. The promotion in
ImplicitTypeCasts does not reach
+ // here because the expected type is a TypeCollection rather than a plain
string type.
Review Comment:
You are right, and the comment taught the wrong invariant:
`expectsStringType` recurses into `TypeCollection`, so `Reverse`'s string
branch does get promoted by `ImplicitTypeCasts`. Reworded to say the override
covers the paths that do not go through implicit casting (an expression built
directly), rather than claiming promotion cannot reach it.
--
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]
