spark-test commented on code in PR #59895:
URL: https://github.com/apache/spark/pull/59895#discussion_r4417403434
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/GroupBasedMergeIntoTableSuite.scala:
##########
@@ -82,6 +82,58 @@ class GroupBasedMergeIntoTableSuite extends
MergeIntoTableSuiteBase {
}
}
+ test("SPARK-60667: merge with type coercion handles metadata columns
correctly") {
Review Comment:
**Nit (P3):** This test and its two delta-based counterparts all combine
`WHEN MATCHED ... UPDATE`, `WHEN NOT MATCHED ... INSERT` and `WHEN NOT MATCHED
BY SOURCE ... DELETE`, so they only reach the full `ReplaceData`/`WriteDelta`
branch of `RewriteMergeIntoTable`. The skip also fixes the two insert-only
branches, which handle a MERGE with only `NOT MATCHED` actions. Those get
target metadata differently. They build `Join(source, r, LeftAnti, cond)` over
the bare target without `buildRelationWithAttrs`, so a target metadata column
in `ON` is supplied only when `AddMetadataColumns` widens that join one
iteration later. For example:
```sql
MERGE INTO t USING s
ON t.pk = s.pk AND t._partition = 'hr' AND t.salary < 600.5
WHEN NOT MATCHED THEN INSERT (pk, salary, dep) VALUES (s.pk, 0, 'new')
```
On master this fails alignment with `No assignment for 'index'`, like the
other MERGE forms the description lists. With this PR it is aligned over the
data-only target and rewritten into `InsertOnlyMerge`. I traced that from
source but did not run it.
No test covers this path. The only insert-only MERGE with a metadata column
in `ON` is the SPARK-60708 char/varchar test in `MergeIntoTableSuiteBase`. It
needs no coercion and is rewritten in the first iteration, so
`AddMetadataColumns` never sees the resolved MERGE. Suppose a later change
narrowed the `MergeIntoTable` arm of `isRowLevelCommandTarget` to MERGEs that
become `ReplaceData` or `WriteDelta`, on the reasoning that an insert-only
MERGE is just an append. That would bring the failure above back while every
test still passed. For a connector whose metadata columns are all nullable, the
append would instead receive them as data columns.
The analyzer change itself handles this case; only the test is missing.
Could you add an insert-only case to `MergeIntoTableSuiteBase`, next to the
SPARK-60708 test, so that it runs for both group-based and delta-based tables?
**Verification:**
- **Regression:** An insert-only MERGE whose ON condition references a
target metadata column and needs type coercion succeeds, and appends exactly
the expected unmatched source rows. The same test must fail if metadata columns
are again appended to the MERGE target before the rewrite. On the in-memory
table, whose metadata columns are non-nullable, that failure is the pre-fix `No
assignment for 'index'` alignment error.
- **Behavior:** The write schema of the appended rows contains only the
table's data columns. Read it from the executed InsertOnlyMergeExec's write, as
the SPARK-58007 insert-only fast-path test in MergeIntoTableSuiteBase does. The
in-memory row-level table records lastWriteInfo and lastWriteLog only for
row-level operation writes, so checkLastWriteInfo and checkLastWriteLog cannot
observe this append.
--
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]
