dongjoon-hyun commented on code in PR #59895:
URL: https://github.com/apache/spark/pull/59895#discussion_r4408133073
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1139,9 +1139,12 @@ class Analyzer(
if (metaCols.isEmpty) {
node
} else {
- val newNode = node.mapChildren(
- addMetadataCol(_, metaCols.map(_.exprId).toSet,
onlyUniqueAndNecessaryMetadataColumns)
- )
+ val requiredAttrIds = metaCols.map(_.exprId).toSet
+ val newNode = node.mapChildren {
+ case child if isRowLevelCommandTarget(node, child) => child
Review Comment:
Thank you for the review, @szehon-ho. You're right. I verified your example
on the in-memory tables without requested metadata (`pk INT NOT NULL, id INT,
dep STRING` with the rows `(1, 1, hr)`, `(2, 2, hr)` and `(3, 1, software)`):
| | `_partition = 'hr' AND id < 1.5` | `_partition = 'hr' AND id < 2` |
|---|---|---|
| master | Succeeds | Fails with `MISSING_ATTRIBUTES` |
| this PR | Fails with `MISSING_ATTRIBUTES` | Fails with
`MISSING_ATTRIBUTES` |
| this PR with #59900 | Succeeds | Succeeds |
The second form already fails on master because the rewrite reads only the
metadata columns that the operation requests (`requiredMetadataAttributes()`
and `rowId()`). The first form succeeded only because `AddMetadataColumns`
added every metadata column to the target in the later iteration. A delta-based
`DELETE` wrote only the row ID, but a group-based `DELETE` also wrote `index`
and `_partition` as data columns.
So I opened #59900 (SPARK-60673) to fix the root cause first. It makes the
rewrite read the metadata columns that the command references without writing
them, and it covers the `id < 2` form. With it on top of this PR, both forms
succeed for group-based and delta-based tables, and the write schemas don't
include the predicate-only metadata (`struct<>` with the row ID `pk` for delta,
`struct<pk:int,id:int,dep:string>` for group). This PR should be merged after
#59900, so I'll rebase it once #59900 is merged and add your `id < 1.5` form
here next to the `id < 2` tests, with the same write schema checks.
--
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]
