cloud-fan commented on code in PR #59271:
URL: https://github.com/apache/spark/pull/59271#discussion_r4232446762
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -1759,15 +1759,26 @@ case class Unpivot(
def canBeCoercioned: Boolean = values.exists(_.nonEmpty) &&
values.exists(_.forall(_.forall(_.resolved))) &&
+ // values are coerced by position, so every value group must have one
value per value column,
+ // otherwise CheckAnalysis reports UNPIVOT_VALUE_SIZE_MISMATCH
+ values.exists(_.forall(_.length == valueColumnNames.length)) &&
// when no ids are given, values must be Attributes (column names) to
allow detecting ids
// coercion will add aliases, would disallow detecting ids, so defer
coercion after id detection
ids.exists(_.forall(_.resolved))
- def valuesTypeCoercioned: Boolean = canBeCoercioned &&
- // all inner values at position idx must have the same data type
+ def valuesTypeCoercioned: Boolean = canBeCoercioned && {
+ // all inner values at position idx must have exactly the same data type,
including nested
Review Comment:
**Nit (P3):** This comment describes exact equality as an unconditional
invariant, but the legacy branch below deliberately uses
DataTypeUtils.sameType, which ignores nested nullability. In that mode
valuesTypeCoercioned can therefore be true for types that are not exactly
equal, so the current wording gives maintainers the wrong contract.
**Verification:**
- **Inspection:** The predicate documentation accurately states both
configured comparison modes and does not imply that the legacy sameType result
is exact equality.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -1759,15 +1759,26 @@ case class Unpivot(
def canBeCoercioned: Boolean = values.exists(_.nonEmpty) &&
values.exists(_.forall(_.forall(_.resolved))) &&
+ // values are coerced by position, so every value group must have one
value per value column,
+ // otherwise CheckAnalysis reports UNPIVOT_VALUE_SIZE_MISMATCH
+ values.exists(_.forall(_.length == valueColumnNames.length)) &&
// when no ids are given, values must be Attributes (column names) to
allow detecting ids
// coercion will add aliases, would disallow detecting ids, so defer
coercion after id detection
ids.exists(_.forall(_.resolved))
- def valuesTypeCoercioned: Boolean = canBeCoercioned &&
- // all inner values at position idx must have the same data type
+ def valuesTypeCoercioned: Boolean = canBeCoercioned && {
+ // all inner values at position idx must have exactly the same data type,
including nested
+ // nullability, because the value column takes the data type of the first
value
+ val sameType: (DataType, DataType) => Boolean =
+ if
(conf.getConf(SQLConf.LEGACY_UNPIVOT_USE_FIRST_VALUE_NESTED_NULLABILITY)) {
+ DataTypeUtils.sameType
+ } else {
+ _ == _
Review Comment:
**Non-blocking (P2):** This also inserts runtime Casts when only nested
nullability differs. SimplifyCasts leaves struct and deeper collection cases
intact, and Cast's identity check still compares nullability, so every affected
Expand branch rebuilds the full complex value per row. For wide structs or
large nested collections that adds payload-linear CPU and allocation even
though the physical representation and contents are unchanged.
**Recommended change:** Treat casts whose source and target complex types
are structurally identical when nullability is ignored as
representation-preserving in both interpreted evaluation and code generation,
while retaining the Cast node and its widened target DataType; add
expression-level coverage for structs and recursively nested arrays and maps.
**Why this works:** Extend Cast's structural identity test to ignore
nullability for physical evaluation. The interpreted cast function returns the
input complex value, and the code-generated path emits the child expression
directly, while the unresolved and analyzed Cast expression continues to
advertise the wider target type to UnpivotTransformer and downstream consumers.
**Scope:** Keep nullability-only Casts as logical type declarations but make
their physical evaluation an identity operation.
**Compatibility:** Default UNPIVOT continues to merge nested nullability and
return nested NULL values correctly, and the legacy switch continues to
preserve first-value nested nullability.
**Risks:** The Cast-level optimization applies beyond UNPIVOT, so coverage
must confirm that forwarding the physical value is safe for every complex
container shape and for both interpreted and generated execution. A too-broad
structural predicate could suppress a real nested conversion or an
ANSI/try-cast failure.
**Constraints:** Apply the identity path only when source and target are
structurally equal after ignoring nullability; field, element, key, or value
type conversions must still execute. Preserve the target DataType and nullable
contract even when evaluation forwards the original physical value. Keep
legacy, ANSI, and try-cast error behavior unchanged for conversions that can
fail.
**Success:** Nullability-only complex casts retain their target logical
DataType without rebuilding InternalRow, ArrayData, or MapData values.
Wide-struct and deep-collection UNPIVOT plans no longer add payload-linear copy
work solely to widen nested nullability. Casts that change an actual nested
data type continue to perform conversion and preserve existing failure
semantics.
**Verification:**
- **Regression:** Nullability-only casts of structs and recursively nested
arrays and maps evaluate without allocating replacement complex containers in
both interpreted and generated paths.
- **Behavior:** The forwarded values remain readable under the widened
target schema and retain correct NULL contents.
- **Compatibility:** Representative complex casts with real nested type
conversions and failure cases continue to convert or fail exactly as before.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -1759,15 +1759,26 @@ case class Unpivot(
def canBeCoercioned: Boolean = values.exists(_.nonEmpty) &&
values.exists(_.forall(_.forall(_.resolved))) &&
+ // values are coerced by position, so every value group must have one
value per value column,
+ // otherwise CheckAnalysis reports UNPIVOT_VALUE_SIZE_MISMATCH
+ values.exists(_.forall(_.length == valueColumnNames.length)) &&
// when no ids are given, values must be Attributes (column names) to
allow detecting ids
// coercion will add aliases, would disallow detecting ids, so defer
coercion after id detection
ids.exists(_.forall(_.resolved))
- def valuesTypeCoercioned: Boolean = canBeCoercioned &&
- // all inner values at position idx must have the same data type
+ def valuesTypeCoercioned: Boolean = canBeCoercioned && {
+ // all inner values at position idx must have exactly the same data type,
including nested
+ // nullability, because the value column takes the data type of the first
value
+ val sameType: (DataType, DataType) => Boolean =
+ if
(conf.getConf(SQLConf.LEGACY_UNPIVOT_USE_FIRST_VALUE_NESTED_NULLABILITY)) {
+ DataTypeUtils.sameType
+ } else {
+ _ == _
Review Comment:
**Non-blocking (P2):** For struct values that differ only in nested
nullability, this comparison now activates common-type coercion.
findTypeForComplex rebuilds the target StructFields through the metadata-free
add overload, and UnpivotTransformer exposes that rebuilt type, so nested
comments and custom metadata from the first value are silently lost even though
the field contents are unchanged.
**Verification:**
- **Regression:** Dataset.unpivot over metadata-bearing struct columns
preserves the selected nested metadata while widening nullable fields,
including both value orders.
- **Behavior:** Struct, array, and map nested-nullability results and NULL
values remain correct in default mode.
- **Compatibility:** The legacy configuration continues to derive nested
nullability from the first value.
--
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]
