dongjoon-hyun commented on PR #58419: URL: https://github.com/apache/spark/pull/58419#issuecomment-5528130225
Summary of my review comments so far, for tracking. **Addressed (thanks for the quick turnaround)** - `doExecuteColumnar` concatenating while a non-plain union advertised `HashPartitioning` ([thread](https://github.com/apache/spark/pull/58419#discussion_r3914971055)): resolved upstream by SPARK-59141 (#58445), now included via the master merge. Both paths share `unionRDDs`. - Double-checked locking in `isPlainUnion` ([thread](https://github.com/apache/spark/pull/58419#discussion_r3914971124)): collapsed to a single `synchronized` block. - PR-history wording in the `outputPartitioning` / memo comments ([thread](https://github.com/apache/spark/pull/58419#discussion_r3914971133), [thread](https://github.com/apache/spark/pull/58419#discussion_r3914971085)): reworded to describe current behavior. The memo comment now correctly says only the `isPlainUnion` term is latched; the pre-existing crash through the codegen confs remains, but that is out of scope here. - Tests ([thread](https://github.com/apache/spark/pull/58419#discussion_r3914971140), [thread](https://github.com/apache/spark/pull/58419#discussion_r3914971154), [thread](https://github.com/apache/spark/pull/58419#discussion_r3914971166)): the two fused-union tests are merged, the unreachable `isCached` guard is gone, and the conf-flip test asserts the literal `(k, 8)` rows. **Still open, waiting for a reply** 1. `CoalesceShufflePartitions.childrenNeedCompatiblePartitioning` reads the latched `isPlainUnion`, so after `OptimizeSkewInRebalancePartitions` splits only one child, both children lose partition coalescing. This is a behavior change introduced by this PR and is neither mentioned in the description nor tested ([thread](https://github.com/apache/spark/pull/58419#discussion_r3914971073)). 2. Reading `outputPartitioning` on the un-prepared `queryExecution.sparkPlan` now latches `plain`, which `executedPlan` inherits through `sparkPlan.clone()` -> `copyTagsFrom`, losing SPARK-52921's exchange elimination for that DataFrame ([thread](https://github.com/apache/spark/pull/58419#discussion_r3914971098)). 3. `@transient private val decisionLock` is `null` after deserialization; `@transient private lazy val` would avoid the NPE ([thread](https://github.com/apache/spark/pull/58419#discussion_r3914971107)). 4. Design: a field forwarded in `withNewChildrenInternal` (and a planner-set constructor field for `UNION_OUTPUT_PARTITIONING`) instead of a `TreeNodeTag` plus a dedicated lock. Fine either way, but the reasoning should be on record ([thread](https://github.com/apache/spark/pull/58419#discussion_r3914971116)). Item 1 is the one I'd like settled before this merges. The Kafka failure in the latest CI run is unrelated (`AvailableNow` partition-metadata timeout); `sql - slow tests` was cancelled and has no result. -- 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]
