cloud-fan commented on code in PR #59271:
URL: https://github.com/apache/spark/pull/59271#discussion_r4250707597
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -1759,15 +1759,28 @@ 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 the same data type; the
value column takes the
+ // data type of the first value, so by default the types must be exactly
equal, including
+ // nested nullability; the legacy mode ignores nested nullability, so the
value column then
+ // takes the nested nullability of the first value
+ val sameType: (DataType, DataType) => Boolean =
+ if
(conf.getConf(SQLConf.LEGACY_UNPIVOT_USE_FIRST_VALUE_NESTED_NULLABILITY)) {
Review Comment:
**Non-blocking (P2):** This branch controls the fixed-point coercion
decision, but the dedicated `UnpivotResolver` calls `UnpivotTypeCoercion`
unconditionally before evaluating `valuesTypeCoercioned`. When single-pass
resolution and this legacy setting are both enabled, the first value has
already been widened, so the result still uses merged nested nullability
instead of the documented first-value behavior.
**Verification:**
- **Regression:** Forced pure single-pass resolution with the legacy flag
returns the first value's nested nullability for struct, array, and map shapes.
- **Compatibility:** Fixed-point and pure single-pass analyzers agree under
both values of the new legacy configuration.
- **Behavior:** Default single-pass resolution continues to merge
nullability and still widens genuinely different value data types.
##########
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:
The wider-type helper predates this PR, but this input did not reach it on
the merge target: sameType treated the nested-nullability difference as already
coerced. The new exact comparison activates metadata-free StructType
reconstruction on the default fixed-point path. Please preserve the selected
nested metadata and add coverage for both value orders.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4232446772","thread_id":"inline:4232446772","verdict_sha256":"ab80a89faa941411863a596e1b8a214971d0adf3472890bfd7cd3a5de803ca85"}
-->
##########
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:
The ordinary Cast implementation predates this PR, but the merge target did
not insert it for nullability-only UNPIVOT values because sameType skipped
coercion. The new exact comparison makes struct and recursively nested casts
survive into execution and copy each affected payload per row. Please make
those representation-preserving casts evaluate as identities, or otherwise
avoid inserting the runtime casts.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4232446785","thread_id":"inline:4232446785","verdict_sha256":"ab80a89faa941411863a596e1b8a214971d0adf3472890bfd7cd3a5de803ca85"}
-->
--
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]
