You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "srielau (via GitHub)" <gi...@apache.org> on 2026/08/26 01:14:35 UTC
[PR] [SPARK-59016][SQL] Keep CHAR/VARCHAR through SimplifyCasts, V2 prune, and inventory [spark]
srielau opened a new pull request, #58299:
URL: https://github.com/apache/spark/pull/58299
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'common/utils/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
[SPARK-59016](https://issues.apache.org/jira/browse/SPARK-59016) (parent [SPARK-58794](https://issues.apache.org/jira/browse/SPARK-58794)).
Follow-up on first-class CHAR/VARCHAR (`spark.sql.charVarchar.standardSemantics.enabled`):
- `SimplifyCasts` keeps `CAST(... AS STRING)` when the child is first-class CHAR/VARCHAR, not only when it is annotated STRING (flag off). Those casts are a type change, not a no-op.
- `PushDownUtils.toOutputAttrs` no longer rewrites CHAR/VARCHAR to annotated STRING when standard semantics are on, so V2 column prune keeps the logical types.
- The FunctionRegistry leak inventory covers extra arities and nested array/map/struct shapes. True pass-throughs (`flatten`, `map_concat`, `map_entries`, `map_keys`, `map_values`, `nvl2`) are allowlisted.
### Why are the changes needed?
With first-class types, `CharType`/`VarcharType` are subclasses of `StringType` but are not unconstrained STRING. Dropping `CAST(c AS STRING)` or stripping length in V2 scan output hides that distinction. The previous inventory only used seven argument templates, so a leak at another arity or nested shape would not fail CI.
### Does this PR introduce _any_ user-facing change?
Yes, only when `spark.sql.charVarchar.standardSemantics.enabled` is true (still off by default).
- `CAST(char_or_varchar AS STRING)` stays in the optimized plan.
- V2 scans that prune columns keep CHAR/VARCHAR in the output schema instead of annotated STRING.
### How was this patch tested?
- `catalyst/testOnly org.apache.spark.sql.catalyst.optimizer.SimplifyCastsSuite -- -z SPARK-59016`
- `sql/testOnly org.apache.spark.sql.BasicCharVarcharTestSuite -- -z inventoried`
- `sql/testOnly org.apache.spark.sql.DSV2CharVarcharTestSuite -- -z prune`
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor Grok 4.6
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59016][SQL] Keep CHAR/VARCHAR through SimplifyCasts, V2 prune, and inventory [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #58299:
URL: https://github.com/apache/spark/pull/58299#discussion_r3863795384
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1243,43 +1243,70 @@ 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.
+ // Pass-through and container functions that may keep CHAR(n)/VARCHAR(n): aggregates
+ // and ordering that return an input unchanged, null-handling, element access,
+ // array/map/struct constructors, and collection rearrangements that keep element types.
+ // Legitimacy is still per shape: reverse(array(c)) may keep CHAR, reverse(c) must not.
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") {
- val argumentShapes = Seq(
- "%s(c)", "%s(c, c)", "%s(c, 'x')", "%s('x', c)", "%s(c, 1)", "%s(array(c))",
- "%s(array(c), '-')")
+ // String-transforming shapes of otherwise pass-through functions. These must reduce to
+ // unconstrained STRING even though the same function keeps CHAR on collection inputs.
+ private val charVarcharTransformingCalls = Set(
+ "concat(c)", "concat(c, c)", "concat(c, c, c)", "concat(c, 'x')", "concat('x', c)",
+ "reverse(c)")
+
+ private val inventoryScalarShapes = Seq(
+ "%s(c)", "%s(c, c)", "%s(c, 'x')", "%s('x', c)", "%s(c, 1)", "%s(array(c))",
+ "%s(array(c), '-')")
+
+ private val inventoryNestedShapes = Seq(
+ "%s(c, c, c)", "%s(array(array(c)))", "%s(named_struct('x', c))",
+ "%s(map(c, 1))", "%s(map(1, c))")
+ private def inventoriedCharVarcharLeaks(argumentShapes: Seq[String]): Seq[String] = {
+ FunctionRegistry.functionSet.map(_.funcName).toSeq.sorted.flatMap { name =>
+ argumentShapes.map(_.format(name)).filter { call =>
+ // Most shapes do not typecheck for a given function; those are simply not evidence.
+ val keepsCharVarchar =
+ Try(sql(s"SELECT $call AS r FROM std_inventory").schema.head.dataType)
+ .toOption
+ .exists(CharVarcharUtils.hasCharVarchar)
+ keepsCharVarchar &&
+ (!charVarcharPassThroughFunctions.contains(name) ||
+ charVarcharTransformingCalls.contains(call))
+ }
+ }
+ }
+
+ test("SPARK-58794: inventoried shapes do not leak CHAR/VARCHAR under standardSemantics") {
withTable("std_inventory") {
sql("CREATE TABLE std_inventory (c CHAR(5)) USING parquet")
withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
- val leaks = FunctionRegistry.functionSet.map(_.funcName).toSeq.sorted
- .filterNot(charVarcharPassThroughFunctions.contains)
- .flatMap { name =>
- argumentShapes.map(_.format(name)).filter { call =>
- // Most shapes do not typecheck for a given function; those are simply not evidence.
- Try(sql(s"SELECT $call AS r FROM std_inventory").schema.head.dataType)
- .toOption
- .exists(CharVarcharUtils.hasCharVarchar)
- }
- }
+ val leaks = inventoriedCharVarcharLeaks(inventoryScalarShapes)
+ assert(leaks.isEmpty,
+ "these inventoried calls returned a CHAR/VARCHAR type; either fix the expression to " +
+ "return plain STRING, add the function to charVarcharPassThroughFunctions, or " +
+ "remove it from charVarcharTransformingCalls: " + leaks.mkString(", "))
Review Comment:
If the failing call is in `charVarcharTransformingCalls`, the only correct fix is to make the expression return STRING. Removing it from that set while the function stays in `charVarcharPassThroughFunctions` would hide a scalar leak (`concat(c)` / `reverse(c)`).
Suggest splitting the message: pass-through names vs transforming calls.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59016][SQL] Keep CHAR/VARCHAR through SimplifyCasts, V2 prune, and inventory [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #58299:
URL: https://github.com/apache/spark/pull/58299#discussion_r3863856618
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1243,43 +1243,70 @@ 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.
+ // Pass-through and container functions that may keep CHAR(n)/VARCHAR(n): aggregates
+ // and ordering that return an input unchanged, null-handling, element access,
+ // array/map/struct constructors, and collection rearrangements that keep element types.
+ // Legitimacy is still per shape: reverse(array(c)) may keep CHAR, reverse(c) must not.
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") {
- val argumentShapes = Seq(
- "%s(c)", "%s(c, c)", "%s(c, 'x')", "%s('x', c)", "%s(c, 1)", "%s(array(c))",
- "%s(array(c), '-')")
+ // String-transforming shapes of otherwise pass-through functions. These must reduce to
+ // unconstrained STRING even though the same function keeps CHAR on collection inputs.
+ private val charVarcharTransformingCalls = Set(
+ "concat(c)", "concat(c, c)", "concat(c, c, c)", "concat(c, 'x')", "concat('x', c)",
+ "reverse(c)")
+
+ private val inventoryScalarShapes = Seq(
+ "%s(c)", "%s(c, c)", "%s(c, 'x')", "%s('x', c)", "%s(c, 1)", "%s(array(c))",
+ "%s(array(c), '-')")
+
+ private val inventoryNestedShapes = Seq(
+ "%s(c, c, c)", "%s(array(array(c)))", "%s(named_struct('x', c))",
+ "%s(map(c, 1))", "%s(map(1, c))")
+ private def inventoriedCharVarcharLeaks(argumentShapes: Seq[String]): Seq[String] = {
+ FunctionRegistry.functionSet.map(_.funcName).toSeq.sorted.flatMap { name =>
+ argumentShapes.map(_.format(name)).filter { call =>
+ // Most shapes do not typecheck for a given function; those are simply not evidence.
+ val keepsCharVarchar =
+ Try(sql(s"SELECT $call AS r FROM std_inventory").schema.head.dataType)
+ .toOption
+ .exists(CharVarcharUtils.hasCharVarchar)
+ keepsCharVarchar &&
+ (!charVarcharPassThroughFunctions.contains(name) ||
+ charVarcharTransformingCalls.contains(call))
+ }
+ }
+ }
+
+ test("SPARK-58794: inventoried shapes do not leak CHAR/VARCHAR under standardSemantics") {
withTable("std_inventory") {
sql("CREATE TABLE std_inventory (c CHAR(5)) USING parquet")
withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
- val leaks = FunctionRegistry.functionSet.map(_.funcName).toSeq.sorted
- .filterNot(charVarcharPassThroughFunctions.contains)
- .flatMap { name =>
- argumentShapes.map(_.format(name)).filter { call =>
- // Most shapes do not typecheck for a given function; those are simply not evidence.
- Try(sql(s"SELECT $call AS r FROM std_inventory").schema.head.dataType)
- .toOption
- .exists(CharVarcharUtils.hasCharVarchar)
- }
- }
+ val leaks = inventoriedCharVarcharLeaks(inventoryScalarShapes)
+ assert(leaks.isEmpty,
+ "these inventoried calls returned a CHAR/VARCHAR type; either fix the expression to " +
+ "return plain STRING, add the function to charVarcharPassThroughFunctions, or " +
+ "remove it from charVarcharTransformingCalls: " + leaks.mkString(", "))
Review Comment:
Fixed in 0b1f49576a1. The helper now partitions failures and emits separate assertions: unknown legitimate pass-throughs direct the reader to `charVarcharPassThroughFunctions`, while transforming calls only instruct them to fix the expression to return plain STRING.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59016][SQL] Keep CHAR/VARCHAR through SimplifyCasts, V2 prune, and inventory [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #58299:
URL: https://github.com/apache/spark/pull/58299#discussion_r3863682392
##########
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.
##########
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:
Restored SPARK-58794 for the original scalar shapes and added SPARK-59016 for the nested/map/struct templates. The function allowlist is now paired with `charVarcharTransformingCalls` so string `reverse(c)` / `concat(c, ...)` cannot hide behind collection pass-throughs of the same function.
##########
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:
Dropped the prune test with the `toOutputAttrs` special-case. The type assertion could pass without that change because first-class types already survive `replaceCharVarcharWithStringInSchema`. A width/preserve-only/flag-off matrix would not have been testing a real prune-path bug.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59016][SQL] Expand CHAR/VARCHAR leak inventory to nested shapes [spark]
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #58299:
URL: https://github.com/apache/spark/pull/58299#issuecomment-5428836214
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/9f99c395ef1a8c89f884e63e072fa0111dccecd0
- merged into branch-4.x https://github.com/apache/spark/commit/1adffbc591cdbdcd53ef656689a6895bd15e7e05
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59016][SQL] Expand CHAR/VARCHAR leak inventory to nested shapes [spark]
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor closed pull request #58299: [SPARK-59016][SQL] Expand CHAR/VARCHAR leak inventory to nested shapes
URL: https://github.com/apache/spark/pull/58299
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59016][SQL] Keep CHAR/VARCHAR through SimplifyCasts, V2 prune, and inventory [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59016][SQL] Keep CHAR/VARCHAR through SimplifyCasts, V2 prune, and inventory [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org