cloud-fan commented on code in PR #58831:
URL: https://github.com/apache/spark/pull/58831#discussion_r4407365810
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveInsertionBase.scala:
##########
@@ -52,9 +53,9 @@ abstract class ResolveInsertionBase extends Rule[LogicalPlan]
{
case (input: StructType, expected: StructType) =>
// Rename inner fields of the input column to pass the by-name
INSERT analysis.
Alias(Cast(queryOutputCol, renameFieldsInStruct(input, expected)),
resolvedCol.name)()
- case (input: ArrayType, expected: ArrayType) =>
+ case (input: ArrayType, expected: ArrayType) if
resolveNestedFieldsByPosition =>
Review Comment:
**Non-blocking (P2):** With the flag set to `false`, this guard, the one on
the map case below, and the two in `renameFieldsInType` stop renaming structs
inside arrays and maps, so those structs match by name again. That is the code
before SPARK-58816. The merge target renames them positionally with no flag,
and SPARK-58816 is already on branch-4.3 and branch-4.x. A user who turns the
flag off to back out only the new unequal-arity behavior also gets back the
silent wrong-field writes SPARK-58816 fixed. For example,
`array(named_struct('y', 20, 'x', 10))` into `ARRAY<STRUCT<x, y>>` writes
`x=10, y=20`, where the merge target writes `x=20, y=10`. The new
`SQLInsertTestSuite` test pins that result. The same applies to map keys and
values, and to arrays or maps nested inside a struct column. No flag value
reproduces the merge-target behavior.
Dropping the flag from the four array and map cases, and keeping it only on
the arity condition in `renameFieldsInStruct`, would make `false` restore
exactly the merge-target behavior. The config doc and the flag-off test's array
and map expectations would change to match.
**Verification:**
- **Behavior:** With
spark.sql.insertInto.columnList.nestedFieldsResolveByPosition.enabled=false, an
equal-arity struct with reordered field names resolves positionally inside an
array element, inside a map value, and inside an array nested in a struct
column; for example array(named_struct('y', 20, 'x', 10)) into ARRAY<STRUCT<x,
y>> writes x=20, y=10.
- **Compatibility:** With the flag false, unequal-arity structs at top level
and inside arrays and maps still resolve by name, so named_struct('y', 1, 'x',
2, 'z', 3) into STRUCT<x, y> with schema evolution writes {x: 2, y: 1, z: 3},
and an equal-arity top-level struct keeps its positional result.
- **Regression:** With the flag true, the positional results for wider,
narrower, and collection inputs are unchanged.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveInsertionBase.scala:
##########
@@ -64,25 +65,27 @@ abstract class ResolveInsertionBase extends
Rule[LogicalPlan] {
}
private def renameFieldsInStruct(input: StructType, expected: StructType):
StructType = {
- if (input.length == expected.length) {
+ if (input.length == expected.length || resolveNestedFieldsByPosition) {
val newFields = input.zip(expected).map { case (f1, f2) =>
f1.copy(name = f2.name, dataType = renameFieldsInType(f1.dataType,
f2.dataType))
}
- StructType(newFields)
+ StructType(newFields ++ input.drop(expected.length))
Review Comment:
Thanks for tracing both paths. On `ACCEPT_ANY_SCHEMA`: the connector does
own schema alignment, but here the duplicate names come from Spark's own
positional rename, not from the user's query. The connector receives `STRUCT<x,
x, z>` for a struct whose field names were unique, and it cannot tell which `x`
the user meant. On where the fix belongs: extra validation or reordering in
schema evolution would stop the leftover `s.z`, but it would still hand the
duplicate-named struct to `ACCEPT_ANY_SCHEMA` connectors, and it would affect
every schema-evolution write rather than only column-list inserts. On error
precedence: a check limited to collisions the rename creates keeps
`INCOMPATIBLE_DATA_FOR_TABLE.AMBIGUOUS_COLUMN_NAME` for the inserts that hit it
today. It only changes which error is reported when a statement that already
fails also has another incompatibility. Since the duplicate names come from
this PR's default-on rename, could the check go into `renameFieldsInStruct` in
thi
s PR rather than a follow-up?
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4385327697","thread_id":"inline:4385327697","verdict_sha256":"a7c3f790e62651fdb1c068c6fb250ef7e26f159546f4b9574e8663002c51bb67"}
-->
--
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]
