You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "xupefei (via GitHub)" <gi...@apache.org> on 2026/05/19 12:23:35 UTC
[PR] [SPARK-56942][SQL] Widen DSv2 row-id resolution to support nested columns [spark]
xupefei opened a new pull request, #55981:
URL: https://github.com/apache/spark/pull/55981
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
DSv2 connectors that implement SupportsDelta currently must use a top-level column for `rowId()`. If a connector returns a multi-segment field reference (e.g. a nested struct field, or `_metadata.row_index` on a file-source-backed table), analysis fails with a ClassCastException because Spark calls `V2ExpressionUtils.resolveRefs[AttributeReference]`, while nested references resolve to `Alias(GetStructField(...))`.
Widen `RewriteRowLevelCommand.resolveRowIdAttrs` and `WriteDelta.rowIdAttrsResolved` to resolve as `NamedExpression` and flatten back via `.toAttribute`. This supports both flat and nested row-id columns; flat-column behavior is unchanged.
Why are the changes needed?
To unblocks Delta Lake DSv2 connectors that identify rows by file-source metadata such as `(_metadata.file_path, _metadata.row_index)`.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New tests are added to make sure it works for nested and flat cases.
### Was this patch authored or co-authored using generative AI tooling?
Yes, generated by Claude.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56942][SQL] Widen DSv2 row-id resolution to support nested columns [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #55981:
URL: https://github.com/apache/spark/pull/55981#issuecomment-5548131801
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56942][SQL] Widen DSv2 row-id resolution to support nested columns [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #55981:
URL: https://github.com/apache/spark/pull/55981#discussion_r3308218671
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/V2ExpressionUtilsSuite.scala:
##########
@@ -37,4 +37,49 @@ class V2ExpressionUtilsSuite extends SparkFunSuite {
}
assert(exc.message.contains("v2Fun(a) ASC NULLS FIRST is not currently supported"))
}
+
+ test("resolveRefs[NamedExpression] handles nested field references") {
+ val structType = StructType(Seq(StructField("inner", IntegerType, nullable = false)))
+ val structAttr = AttributeReference("outer", structType, nullable = false)()
+ val plan = LocalRelation(structAttr)
+ val nestedRef = FieldReference(Seq("outer", "inner"))
+
+ // A nested reference resolves to an Alias(GetStructField(...)), not an AttributeReference,
+ // so the caller's checkcast fails when it dereferences an element as AttributeReference.
+ // (The `asInstanceOf[T]` inside `resolveRef` is erased to the `NamedExpression` upper bound,
+ // so the CCE fires at the call site, not inside the helper.)
+ intercept[ClassCastException] {
+ val resolvedAsAttr: AttributeReference =
+ V2ExpressionUtils.resolveRefs[AttributeReference](Seq(nestedRef), plan).head
+ resolvedAsAttr
+ }
Review Comment:
The `intercept[ClassCastException]` block locks in a behaviour that's an artefact of Scala generic erasure — the comment correctly explains it, but the assertion documents the *bug* rather than the contract being added. If anyone later changes the helper to do an explicit runtime check (the cleaner shape), this test breaks for unrelated reasons. Drop the regression half; the positive assertion on the `NamedExpression` shape below is what the PR needs to keep passing.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteRowLevelCommand.scala:
##########
@@ -91,9 +91,9 @@ trait RewriteRowLevelCommand extends Rule[LogicalPlan] {
relation: DataSourceV2Relation,
operation: SupportsDelta): Seq[AttributeReference] = {
- val rowIdAttrs = V2ExpressionUtils.resolveRefs[AttributeReference](
+ val rowIdAttrs = V2ExpressionUtils.resolveRefs[NamedExpression](
operation.rowId.toImmutableArraySeq,
- relation)
+ relation).map(_.toAttribute.asInstanceOf[AttributeReference])
Review Comment:
The synthetic `AttributeReference` returned from `.toAttribute` has no link to the underlying scan: for a nested ref like `outer.inner`, the scan still produces `outer`, and nothing extracts `inner` from it. `buildRelationWithAttrs` then folds this synthetic attribute into `relation.output` as if it were a real column.
The structural fix is to keep `resolveRowIdAttrs` returning `Seq[NamedExpression]` and wrap the read relation with a `Project` that aliases each non-`Attribute` rowId expression to a top-level helper column — same pattern as `ResolveChangelogTable.addStreamingNetChangeComputation` (`ResolveChangelogTable.scala:706-710`):
```scala
val rowIdHelpers: Seq[Alias] = rowIdExprs.zipWithIndex.map { case (expr, idx) =>
Alias(expr, NetChangesHelperColumns.rowIdColumn(idx))()
}
val withHelpers = Project(originalCols ++ rowIdHelpers, watermarked)
```
Flat case stays a no-op (no Project inserted, no fresh `ExprId`s); nested case gets a real column with `GetStructField(...)` actually evaluated. `WriteDelta.rowIdAttrsResolved` at `v2Commands.scala:500` is then correct as-is — it's a structural check against the connector's flat `rowIdProjection.schema`. The `.asInstanceOf[AttributeReference]` here and its inconsistency with that site (which omits the cast) both go away once the return type is widened.
Related: `requiredMetadataAttributes()` at line 85 above and `filterAttributes()` at `DataSourceV2Relation.scala:185` are also `NamedReference[]` but still use `resolveRefs[AttributeReference]`. Either widen consistently across the row-level API surface, or document why nested refs are only meaningful for rowIds.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56942][SQL] Widen DSv2 row-id resolution to support nested columns [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #55981: [SPARK-56942][SQL] Widen DSv2 row-id resolution to support nested columns
URL: https://github.com/apache/spark/pull/55981
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org