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: [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]
