spark-test commented on code in PR #59895:
URL: https://github.com/apache/spark/pull/59895#discussion_r4407289992
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1176,6 +1179,27 @@ class Analyzer(
})
}
+ // DELETE, UPDATE and MERGE on a table that supports row-level operations
are rewritten into
+ // plans that read the metadata columns they need (see
RewriteRowLevelCommand). The rewrite
+ // takes the output of the target table as the data columns to write, so
metadata columns must
+ // not be added to it, even if the command is resolved only in a later
iteration. This does not
+ // apply to UPDATE and MERGE on a table that accepts any schema, which
Spark leaves to the rules
Review Comment:
**Nit (P3):** This reason is broader than what Spark defers. For tables that
accept any schema, Spark leaves only assignment *alignment* to the injected
rules (`ResolveRowLevelCommandAssignments` skips them); the rewrite itself is
still Spark's. A MERGE whose actions are all `DELETE` has nothing to align
(`MergeIntoTable.aligned` is trivially true), and `RewriteMergeIntoTable`
rewrites any `resolved && rewritable && aligned` MERGE without looking at
`skipSchemaResolution`. For example, on an `ACCEPT_ANY_SCHEMA` row-level table:
```sql
MERGE INTO t USING s ON t.pk = s.pk
WHEN MATCHED AND t._partition = 'hr' AND t.salary < 600.5 THEN DELETE
```
`isRowLevelCommandTarget` returns false here, so in the later iteration
`AddMetadataColumns` still appends the metadata columns to the target, and
Spark's own rewrite takes that output as the data columns to write
(`ReplaceData`/`WriteDelta.rowAttrsResolved` short-circuit on
`skipSchemaResolution`, so nothing rejects it). That runtime behavior is the
same as on master and within the exclusion the PR describes, but the comment
says no Spark rewrite is involved. Could it say that only assignment alignment
is left to the injected rules, and that a MERGE with only DELETE actions on
such a table is still rewritten by Spark and keeps the old widening? If you
would rather fix that case too, the skip could also cover MERGE statements that
Spark rewrites without alignment, but that changes behavior inside the stated
exclusion.
**Verification:**
- **Inspection:** Check that the revised comment matches the code: it should
describe the exclusion as applying to UPDATE and MERGE whose target skips
schema resolution, attribute only assignment alignment to connector-injected
rules, and say that a MERGE with only DELETE actions on such a table is still
rewritten by Spark, because the MERGE rewrite does not check whether the target
skips schema resolution.
- **Inspection:** Check that the change touches only comment text in the
AddMetadataColumns helper, with lines within the 100-character limit and
ASCII-only characters, so the analyzer's behavior is unchanged.
--
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]
