srielau commented on code in PR #58299:
URL: https://github.com/apache/spark/pull/58299#discussion_r3863647324
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/PushDownUtils.scala:
##########
@@ -704,8 +704,12 @@ object PushDownUtils extends Logging {
schema: StructType,
relation: DataSourceV2Relation): Seq[AttributeReference] = {
val nameToAttr = Utils.toMap(relation.output.map(_.name), relation.output)
- val cleaned = CharVarcharUtils.replaceCharVarcharWithStringInSchema(schema)
- toAttributes(cleaned).map {
+ // Under standardSemantics the scan keeps first-class CHAR/VARCHAR.
Flag-off still
+ // rewrites to annotated STRING so ApplyCharTypePadding can find the
original type.
+ val outputSchema =
+ if (SQLConf.get.charVarcharStandardSemantics) schema
+ else CharVarcharUtils.replaceCharVarcharWithStringInSchema(schema)
Review Comment:
Dropped the special-case. `replaceCharVarcharWithString` already keeps
first-class CHAR/VARCHAR (`charVarcharFirstClassTypes`), and
`replaceCharVarcharWithStringInSchema` only stamps `__CHAR_VARCHAR_TYPE_STRING`
on those fields. Skipping the helper only on prune was inconsistent with
`DataSourceV2Relation.create`. Leaving the stamp in place: ApplyCharTypePadding
still uses it as an idempotence marker, so skipping it in the schema helper
would be a broader behavior change than this PR.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:
##########
@@ -1158,8 +1159,13 @@ object FoldablePropagation extends Rule[LogicalPlan] {
object SimplifyCasts extends Rule[LogicalPlan] {
def apply(plan: LogicalPlan): LogicalPlan =
plan.transformAllExpressionsWithPruning(
_.containsPattern(CAST), ruleId) {
+ // Annotated STRING (flag off) and first-class CHAR/VARCHAR are not
unconstrained STRING.
+ // Dropping CAST(... AS STRING) would hide the type change.
case c @ Cast(e: NamedExpression, StringType, _, _)
if e.dataType == StringType &&
e.metadata.contains(CHAR_VARCHAR_TYPE_STRING_METADATA_KEY) => c
+ case c @ Cast(e, dt: StringType, _, _)
+ if CharVarcharUtils.hasCharVarchar(e.dataType) &&
+ !CharVarcharUtils.hasCharVarchar(dt) => c
Review Comment:
Dropped the extra arm. `CharType`/`VarcharType` are not equal to
unconstrained `StringType`, so `e.dataType == dataType` already keeps
`CAST(char AS STRING)`. The annotated-STRING arm stays.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/SimplifyCastsSuite.scala:
##########
@@ -138,4 +140,28 @@ class SimplifyCastsSuite extends PlanTest {
input.select($"a".cast(DecimalType(2, 1)).as("v")).analyze),
input.select($"a".cast(DecimalType(2, 1)).as("v")).analyze)
}
+
+ test("SPARK-59016: do not drop CAST from CHAR/VARCHAR or annotated STRING to
STRING") {
+ def keepsCast(plan: LogicalPlan): Boolean =
+ plan.exists(_.expressions.exists(_.exists(_.isInstanceOf[Cast])))
Review Comment:
Removed the test with the production arm. `keepsCast` would have passed
without the change, and `comparePlans` would not have moved either.
--
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]
