srielau commented on code in PR #58299:
URL: https://github.com/apache/spark/pull/58299#discussion_r3863546762
##########
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:
Should this use `charVarcharFirstClassTypes` rather than
`charVarcharStandardSemantics`?
`replaceCharVarcharWithString` already keeps Char/Varchar when first-class
types are on; `replaceCharVarcharWithStringInSchema` only adds
`__CHAR_VARCHAR_TYPE_STRING` metadata. `DataSourceV2Relation.create` still goes
through that helper, so prune-only skipping is inconsistent with relation
construction.
If the bug is metadata on first-class fields, the skip belongs in
`replaceCharVarcharWithStringInSchema`. If prune really produced annotated
STRING, `scan.readSchema()` was already STRING and skipping the helper cannot
recover CHAR. A fail-before schema would help.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2423,6 +2424,21 @@ class DSV2CharVarcharTestSuite extends
CharVarcharTestSuite
}
}
+ test("SPARK-59016: V2 column prune keeps CHAR/VARCHAR under
standardSemantics") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ withTable("std_v2_prune") {
+ sql(s"CREATE TABLE std_v2_prune (c CHAR(5), v VARCHAR(5), i INT) USING
$format")
+ sql("INSERT INTO std_v2_prune VALUES ('ab', 'cd', 1)")
+ val charDf = sql("SELECT c FROM std_v2_prune WHERE c = 'ab '")
+ assert(charDf.schema.head.dataType === CharType(5))
Review Comment:
Please assert the pruned schema width (one column) and cover preserve-only
plus flag-off (annotated STRING + raw-type metadata). As written this assertion
can pass without the `toOutputAttrs` change, because first-class types already
survive `replaceCharVarcharWithStringInSchema`.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1246,21 +1246,22 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
// Allowlist for the inventory below: pass-through and container cases that
may keep
// CHAR(n)/VARCHAR(n): aggregates/ordering that return an input unchanged,
null-handling,
// element access, array/map/struct constructors, and collection
rearrangements that keep
- // element types. Coverage is limited to the seven fixed argumentShapes
templates in the test;
- // a leak only at another arity or nested shape would not fail here. For
those shapes,
- // anything not listed must reduce to plain STRING.
+ // element types. For the listed argument shapes, anything not listed must
reduce to plain
+ // STRING.
private val charVarcharPassThroughFunctions = Set(
"any_value", "approx_top_k", "approx_top_k_accumulate", "array",
"array_agg", "array_compact",
"array_distinct", "array_max", "array_min", "array_repeat", "array_sort",
"arrays_zip",
"coalesce", "collect_list", "collect_set", "collect_union", "concat",
"explode",
- "explode_outer", "first", "first_value", "get", "greatest", "ifnull",
"last", "last_value",
- "least", "map", "max", "max_by", "measure", "min", "min_by", "mode",
"named_struct", "nullif",
- "nullifzero", "nvl", "reverse", "shuffle", "sort_array", "struct",
"trim_array", "when")
+ "explode_outer", "first", "first_value", "flatten", "get", "greatest",
"ifnull", "last",
+ "last_value", "least", "map", "map_concat", "map_entries", "map_keys",
"map_values", "max",
+ "max_by", "measure", "min", "min_by", "mode", "named_struct", "nullif",
"nullifzero", "nvl",
+ "nvl2", "reverse", "shuffle", "sort_array", "struct", "trim_array", "when")
- test("SPARK-58794: inventoried shapes do not leak CHAR/VARCHAR under
standardSemantics") {
+ test("SPARK-59016: inventoried shapes do not leak CHAR/VARCHAR under
standardSemantics") {
Review Comment:
Please keep this as SPARK-58794 (or add a separate SPARK-59016 test for the
new templates). The allowlist is still per function name while legitimacy is
per shape (`reverse(c)` vs `reverse(array(c))`). Adding nested/map/struct
templates makes that hole larger; exemptions should be shape-aware.
##########
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:
I do not see a failing case for this arm. `StringType.equals` compares
`constraint`, so `CharType(5) != StringType` and the existing `e.dataType ==
dataType` rule already keeps `CAST(char AS STRING)`. The annotated-STRING case
stays necessary because that path is `StringType == StringType`; this new arm
does not subsume it.
Can we drop this branch, or add a fail-before `comparePlans` that actually
changes without it?
##########
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:
`keepsCast` only looks for some `Cast` in the tree, so this passes on
current `SimplifyCasts` without the new production arm (`CharType(5) !=
StringType`). Prefer `comparePlans` against a plan that still contains the
`Cast`.
--
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]
