spark-test commented on code in PR #59930:
URL: https://github.com/apache/spark/pull/59930#discussion_r4418655131
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -9028,6 +9028,18 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val LEGACY_IGNORE_METADATA_COLUMN_ASSIGNMENTS =
+ buildConf("spark.sql.legacy.ignoreMetadataColumnAssignments")
+ .internal()
+ .doc("When true, UPDATE and MERGE on a Data Source V2 table ignore
assignments to " +
Review Comment:
**Nit (P3):** This says the flag makes UPDATE and MERGE ignore assignments
to metadata columns, as in Spark 4.3 and earlier, and the migration note says
'Previously, these assignments were ignored'. That only holds when the command
is aligned in the analyzer iteration that resolves it.
The flag just skips `checkNoMetadataColumnAssignments`. Suppose a command is
first resolved in a later iteration, for example because a condition needs an
implicit cast, as in the PR's own `WHERE id < 1.5` cases. By then
`AddMetadataColumns` has already appended the metadata columns to the target
output, and alignment treats them as target columns and keeps the assigned
value. With the flag set, `UPDATE t SET index = 5 WHERE id < 1.5` keeps `index
= 5`. As the PR description notes for that path, the value then reaches the
writer or the query fails with an internal error. The legacy-mode tests all use
first-iteration statements, so they do not show this.
It would be more accurate to say that the flag skips the new rejection and
restores the earlier behavior, in which such assignments passed analysis and
were usually dropped. The same applies to the migration note and to the comment
above `checkNoMetadataColumnAssignments` ('Assignments are aligned only with
the table attributes'). If apache/spark#59895 lands first and stops the
later-iteration widening, the current wording may become accurate.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59930#issuecomment-6506622790).
**Verification:**
- **Inspection:** The config doc, the migration-guide entry and the comment
on the check describe legacy mode as skipping the rejection and restoring the
earlier behavior, without an unconditional claim that the assignments are
ignored.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AssignmentUtils.scala:
##########
@@ -72,6 +72,8 @@ object AssignmentUtils extends SQLConfHelper with CastSupport
{
val errors = new mutable.ArrayBuffer[String]()
+ checkNoMetadataColumnAssignments(assignments, err => errors += err)
Review Comment:
**Non-blocking (P2):** This check only runs inside alignment, and for MERGE
`ResolveRowLevelCommandAssignments` aligns only once `containsFinalSchema`
holds. By then, `ResolveSchemaEvolution` has already applied the pending table
changes with `catalog.alterTable` earlier in the same analyzer iteration. A key
bound to a metadata column is resolved much earlier, by `ResolveReferences`.
When its type matches the source column it adds no pending change, so it does
not block schema evolution either.
For example, take the in-memory test table (metadata columns `index` and
`_partition`) and a source `(pk, salary, dep, index, bonus)`:
```sql
MERGE WITH SCHEMA EVOLUTION INTO t USING s ON t.pk = s.pk
WHEN MATCHED THEN UPDATE SET *
WHEN NOT MATCHED THEN INSERT *
```
1. In the first analyzer iteration, the `index` key binds to the metadata
column and `bonus` stays unresolved.
2. In the second, `ResolveSchemaEvolution` adds `bonus` through `alterTable`.
3. Then alignment runs and this check throws `Cannot assign to metadata
column 'index'`.
The same happens with an explicit `UPDATE SET bonus = s.bonus, _partition =
s._partition`, or when the evolution is a type widening (for example a BIGINT
`salary` in the source), which is harder to undo.
Without a transactional catalog, the statement fails but the evolved schema
stays committed, and no rows are written. Before this PR the same statement
succeeded and dropped the metadata-named values. The new schema-evolution tests
do not catch this: their only extra source column is metadata-named, so nothing
is evolved before the error, and `testEvolution` does not re-read the target
schema after an expected error.
**Recommended change:** Reject assignments whose key references a target
metadata column as soon as the assignment keys of a rewritable UPDATE or MERGE
are resolved, rather than inside alignment. Do this in a case of the row-level
assignment resolution rule that comes before the alignment cases and does not
wait for the whole command to resolve, for schema evolution to finish, or for
realignment. Keep the legacy flag, the error condition and the message. Add
MERGE schema-evolution tests in which the source also carries a column missing
from the target, asserting that the target's schema and rows are unchanged
after the error.
**Why this works:** ResolveReferences resolves MERGE keys, including
star-expanded ones, earlier in the same analyzer iteration than the row-level
assignment rule. ResolveSchemaEvolution can apply table changes only in a later
iteration, once the values are resolved and the actions are ready for
evolution. A case that fails as soon as any resolved key carries the
metadata-column marker therefore fails analysis before any catalog.alterTable.
Because it does not depend on alignment, it also rejects assignment lists that
are already aligned with a target output that includes metadata columns.
**Scope:** Move the metadata-column key rejection for rewritable UPDATE and
MERGE ahead of schema evolution and realignment, inside the row-level
assignment resolution rule, and pin the ordering with schema-evolution tests.
**Compatibility:** Statements without metadata-column assignment keys,
legacy-mode behavior, ACCEPT_ANY_SCHEMA and connector-handled targets, and the
existing error condition and message stay unchanged.
**Risks:** Rejecting before the whole command is resolved can surface the
metadata-column error ahead of an unrelated resolution error in the same
statement; this changes which error a user sees, not whether the statement
fails. The early rejection reports only metadata-column problems for the
offending command or MERGE action, so other alignment errors in the same action
appear only after the metadata-column assignment is removed. Alignment over a
target widened by AddMetadataColumns emits an assignment for every metadata
column, for example `_partition = _partition` in an UPDATE. The early check
must only inspect assignments the statement itself supplies: if it ever saw a
command Spark had already aligned before the rewrite rules consumed it, it
would reject a statement that only reads metadata columns.
**Constraints:** Apply the early rejection only to the targets alignment
covers today: rewritable DSv2 tables that do not skip schema resolution,
leaving ACCEPT_ANY_SCHEMA and connector-handled targets unaffected. Keep the
legacy flag semantics: with the flag set, there is no rejection and resolution
and alignment are unchanged.
**Exclusions:**
- Making schema evolution defer catalog changes until every analysis check
has passed, which would also cover unrelated alignment errors, is a broader
change outside this PR.
- Correcting the description of legacy mode in the config doc and migration
guide is handled by the separate documentation fix.
**Success:** A MERGE WITH SCHEMA EVOLUTION whose explicit or star-generated
assignments reference a target metadata column fails with the existing
metadata-column error before any table change is applied, so the target schema
and rows are unchanged. Statements without metadata-column assignment keys, and
all statements with the legacy flag set, resolve and execute exactly as in the
current change.
**Verification:**
- **Regression:** A MERGE WITH SCHEMA EVOLUTION from a source that has a
column named like a target metadata column plus another column missing from the
target fails with the metadata-column error, and the target schema and rows are
unchanged afterwards. Cover both UPDATE SET * with INSERT * and an explicit
assignment to the metadata column.
- **Behavior:** The existing rejection cases for UPDATE and MERGE still fail
with the same error condition and message: unqualified and qualified keys,
not-matched-by-source updates, inserts including insert-only MERGE,
later-iteration resolution, and the DataFrame API.
- **Compatibility:** With the legacy flag set, the same schema-evolution
statement still succeeds as before, and MERGE without the evolution clause
still ignores a metadata-named source column.
- **Behavior:** UPDATE and MERGE statements that are resolved in a later
analyzer iteration and reference metadata columns only in conditions or
assignment values, so that AddMetadataColumns widens the target, are not
rejected by the early check and behave as in the current change.
--
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]
