yyanyy commented on code in PR #58086:
URL: https://github.com/apache/spark/pull/58086#discussion_r3809376265
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLInsertTestSuite.scala:
##########
@@ -591,6 +591,50 @@ trait SQLInsertTestSuite extends QueryTest with
AdaptiveSparkPlanHelper {
}
}
}
+
+ test("SPARK-58816: insert with column list resolves structs inside arrays
positionally") {
+ withTable("t") {
+ createTable("t", Seq("arr"), Seq("ARRAY<STRUCT<x: INT, y: INT>>"))
+ // Source has fields in (y, x) order; target expects (x, y).
+ // Positional resolution must rename y->x and x->y so the cast maps by
position.
+ sql("INSERT INTO t (arr) SELECT array(named_struct('y', 20, 'x', 10))")
+ checkAnswer(spark.table("t"), Row(Seq(Row(20, 10))))
+ }
+ }
+
+ test("SPARK-58816: insert with column list resolves structs inside maps
positionally") {
+ withTable("t") {
+ createTable("t", Seq("m"), Seq("MAP<STRING, STRUCT<x: INT, y: INT>>"))
+ sql("INSERT INTO t (m) SELECT map('k', named_struct('y', 20, 'x', 10))")
+ checkAnswer(spark.table("t"), Row(Map("k" -> Row(20, 10))))
+ }
+ }
+
+ test("SPARK-58816: insert with column list resolves structs positionally at
all nesting levels") {
Review Comment:
The test name says "at all nesting levels", but the three cases here
exercise only a direct struct or a single collection layer. Could we add one
representative mixed case with at least two recursive transitions, for example
`ARRAY<STRUCT<nested: ARRAY<STRUCT<x: INT, y: INT>>>>` or `ARRAY<MAP<STRING,
STRUCT<x: INT, y: INT>>>`?
That would fail if the recursive walk stopped after the first `ArrayType`,
`MapType`, or `StructType` boundary. One such case should be enough; I don't
think we need to test every possible combination.
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLInsertTestSuite.scala:
##########
@@ -591,6 +591,50 @@ trait SQLInsertTestSuite extends QueryTest with
AdaptiveSparkPlanHelper {
}
}
}
+
+ test("SPARK-58816: insert with column list resolves structs inside arrays
positionally") {
+ withTable("t") {
+ createTable("t", Seq("arr"), Seq("ARRAY<STRUCT<x: INT, y: INT>>"))
+ // Source has fields in (y, x) order; target expects (x, y).
+ // Positional resolution must rename y->x and x->y so the cast maps by
position.
+ sql("INSERT INTO t (arr) SELECT array(named_struct('y', 20, 'x', 10))")
Review Comment:
Could we add a paired end-to-end regression case for explicit `BY NAME`,
using the same reversed source field order?
The column-list form here should resolve by position and produce `{x: 20, y:
10}`, while
`INSERT INTO t BY NAME SELECT array(named_struct('y', 20, 'x', 10)) AS arr`
should continue to resolve by name and produce `{x: 10, y: 20}`.
SPARK-36498 already has analyzer-level coverage for structs inside arrays
and maps, but those tests primarily assert the resulting schema. A value-level
SQL test here would lock down the semantic boundary and ensure this fix does
not accidentally change explicit `BY NAME` behavior.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveInsertionBase.scala:
##########
@@ -62,16 +66,25 @@ abstract class ResolveInsertionBase extends
Rule[LogicalPlan] {
private def renameFieldsInStruct(input: StructType, expected: StructType):
StructType = {
if (input.length == expected.length) {
val newFields = input.zip(expected).map { case (f1, f2) =>
- (f1.dataType, f2.dataType) match {
- case (s1: StructType, s2: StructType) =>
- f1.copy(name = f2.name, dataType = renameFieldsInStruct(s1, s2))
- case _ =>
- f1.copy(name = f2.name)
- }
+ f1.copy(name = f2.name, dataType = renameFieldsInType(f1.dataType,
f2.dataType))
}
StructType(newFields)
} else {
input
}
}
+
+ // Recursively rename fields so that positional INSERT analysis applies at
every nesting level,
+ // including structs inside arrays and maps. See SPARK-58816.
+ private def renameFieldsInType(input: DataType, expected: DataType):
DataType =
+ (input, expected) match {
+ case (s1: StructType, s2: StructType) =>
+ renameFieldsInStruct(s1, s2)
+ case (ArrayType(e1, n1), ArrayType(e2, _)) =>
+ ArrayType(renameFieldsInType(e1, e2), n1)
+ case (MapType(k1, v1, n1), MapType(k2, v2, _)) =>
Review Comment:
Could we also add coverage for a struct-valued map key? This branch recurses
independently into both the key and value types, while the current map tests
exercise only the value path (`MAP<STRING, STRUCT<...>>`).
A case such as `MAP<STRUCT<x: INT, y: INT>, STRING>` would protect the key
recursion without adding much test complexity.
--
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]
