cloud-fan commented on code in PR #58831:
URL: https://github.com/apache/spark/pull/58831#discussion_r4385327709
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -5903,6 +5903,17 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val INSERT_COLUMN_LIST_NESTED_FIELDS_RESOLVE_BY_POSITION =
Review Comment:
**Nit (P3):** This entry lands between `INSERT_INTO_REPLACE_ON_ENABLED` and
`INSERT_INTO_REPLACE_ON_BY_NAME_ENABLED`, splitting the REPLACE ON/USING/WHERE
syntax flags, which have nothing to do with column-list nested resolution.
Could it move after `INSERT_INTO_REPLACE_WHERE_COLUMN_LIST_ENABLED`, at the end
of that group, with the definition unchanged?
**Verification:**
- **Inspection:** The REPLACE ON/USING/WHERE syntax-flag entries in SQLConf
appear contiguously, and the column-list nested-resolution entry comes after
them with an identical definition.
- **Compile:** The catalyst module still compiles and the config is still
registered under the same key.
##########
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:
**Non-blocking (P2):** With the flag on, this appends
`input.drop(expected.length)` under the source names without checking them
against the target names the prefix was just renamed to. For `s STRUCT<x INT>`
and `named_struct('y', 1, 'x', 2, 'z', 3)`, the cast type becomes `STRUCT<x, x,
z>`.
The PR relies on `TableOutputResolver.reorderColumnsByName` to reject that
with `AMBIGUOUS_COLUMN_NAME`, but two consumers act before or instead of that
check:
- **Schema evolution runs first.** `ResolveSchemaEvolution` runs before
`ResolveOutputRelation`, and `computeSchemaChangesByName` dedups the duplicate
`x` last-wins, so it computes only `add s.z` and calls `catalog.alterTable`.
The ambiguity error fires after that, so the `INSERT WITH SCHEMA EVOLUTION`
fails but leaves `s.z` in the table schema. At the merge base the same
statement succeeded.
- **`ACCEPT_ANY_SCHEMA` tables skip the check.** `skipSchemaResolution`
makes `outputResolved` true, so `ResolveOutputRelation` never runs and the
connector receives `Cast(s, STRUCT<x, x, z>)` with duplicate names the user
never wrote.
The existing collision test (`named_struct('y', 2, 'x', 3)` into
`STRUCT<x>`) has no extra non-colliding field and does not use an
`ACCEPT_ANY_SCHEMA` table, so it observes neither effect. v1/Hive inserts are
not affected because they cannot evolve and always go through output resolution.
**Recommended change:** In the column-list INSERT projection, after renaming
the overlapping prefix of a source struct to the target field names, check
whether any retained surplus source field name matches one of those assigned
names under the session resolver. If one does, fail analysis immediately with
the existing INCOMPATIBLE_DATA_FOR_TABLE.AMBIGUOUS_COLUMN_NAME error for that
nested column path, instead of emitting a cast to a struct with duplicate field
names. Pass the table name and the nested column path through the struct and
nested-type rename helpers, using the same element, key and value path segments
that table output resolution uses for arrays and maps, so the error names the
same column the current tests expect.
**Why this works:** The duplicate-named struct is only ever created by the
positional rename. Rejecting it there means no consumer ever sees it: schema
evolution never computes or applies table changes from it, ACCEPT_ANY_SCHEMA
writes are rejected before the write reaches the connector, and v1/Hive and v2
output resolution report the same error earlier than they do today.
**Scope:** Reject positional-rename name collisions where the renamed struct
is built, and add regression coverage for the schema-evolution, connector and
nested cases.
**Compatibility:** Column-list inserts whose renamed struct has no
collision, including the PR's {y, x, z} into STRUCT<x, y> example, and all
behavior with the compatibility flag off, produce the same results as at the
current head.
**Risks:** The collision is now reported during the column-list projection,
before table output resolution. A statement that has both a collision and
another incompatibility will report the collision first.
**Constraints:** Reject only collisions between a retained surplus source
field and a name assigned by the positional rename. User-written duplicate
names and unequal-arity inputs without such a collision must keep their current
behavior. Compare names with the session resolver so that case sensitivity
follows spark.sql.caseSensitive, matching how table output resolution detects
the ambiguity today.
**Exclusions:**
- Introducing a new, more specific error condition for this collision is out
of scope. The repair keeps INCOMPATIBLE_DATA_FOR_TABLE.AMBIGUOUS_COLUMN_NAME,
which the PR's tests already assert.
**Success:** For a schema-evolving v2 table with s STRUCT<x INT>, INSERT
WITH SCHEMA EVOLUTION INTO t (s) SELECT named_struct('y', 1, 'x', 2, 'z', 3)
fails with INCOMPATIBLE_DATA_FOR_TABLE.AMBIGUOUS_COLUMN_NAME on `s`.`x`, and
the table schema afterwards still has no s.z. A column-list INSERT whose
positional rename produces a colliding surplus name is rejected during analysis
for every table kind, including tables with ACCEPT_ANY_SCHEMA, so no write
receives a struct with duplicate field names it did not contain. A collision in
a struct nested in an array element or a map key or value is rejected with the
corresponding nested column path. Non-colliding inputs, including
named_struct('y', 1, 'x', 2, 'z', 3) into STRUCT<x, y> writing {x: 1, y: 2, z:
3}, and every flag-off behavior are unchanged.
**Verification:**
- **Regression:** A schema-evolving v2 table with s STRUCT<x INT>, given a
column-list insert of named_struct('y', 1, 'x', 2, 'z', 3) WITH SCHEMA
EVOLUTION, fails with AMBIGUOUS_COLUMN_NAME on `s`.`x`, and the table schema is
unchanged afterwards.
- **Regression:** A column-list insert with a colliding surplus name into a
table that reports ACCEPT_ANY_SCHEMA is rejected during analysis instead of
reaching the connector.
- **Behavior:** A colliding surplus name in a struct nested inside an array
element and inside a map value is rejected, and the error names the nested
column path.
- **Compatibility:** The existing collision cases still fail with
AMBIGUOUS_COLUMN_NAME on `s`.`x` with and without schema evolution.
Non-colliding wider and narrower sources keep their positional results with the
flag on and their by-name results with it off.
- **Compatibility:** With case-insensitive analysis, a surplus field
differing from a renamed target name only by case is treated as a collision.
With spark.sql.caseSensitive=true it is not, and it is evolved or rejected as
an extra field as today.
##########
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) {
Review Comment:
**Non-blocking (P2):** Since this condition now renames on any arity
mismatch at every nesting level, a few behaviors it changes or keeps still have
no test:
- **Unequal arity inside a collection.** For example,
`array(named_struct('y', 1, 'x', 2, 'z', 3))` into `ARRAY<STRUCT<x, y>>` with
schema evolution. The PR description claims this case, but the SPARK-58816
array/map tests are all equal-arity and the new tests only use a top-level
struct, so if the relaxation stopped applying to collection elements, every
current test would still pass.
- **A narrower source.** With `spark.sql.insertNestedTypeCoercion.enabled`
and schema evolution, `named_struct('y', 1)` into `STRUCT<x, y>` now writes
`{x: 1, y: null}` instead of `{x: null, y: 1}`. The existing
missing-nested-field tests use `INSERT ... SELECT *` without a column list, so
they never reach this code.
- **Equal arity with the flag off.** The flag-off tests use top-level
array/map columns, which never reach this method, and an unequal-arity struct.
Dropping the `input.length == expected.length ||` disjunct would switch
equal-arity structs to by-name under flag=false without any test failing.
The first item is also the array/map coverage asked for earlier in review.
This extends an [existing
discussion](https://github.com/apache/spark/pull/58831#discussion_r4047775291).
**Verification:**
- **Behavior:** Unequal-arity structs inside an array element and inside a
map value resolve positionally with the flag on and by name with it off, under
schema evolution.
- **Behavior:** A narrower source struct under nested type coercion with
schema evolution resolves positionally with the flag on and by name with it off.
- **Compatibility:** With the flag off, an equal-arity struct with reordered
field names keeps its positional result.
--
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]
