peter-toth commented on PR #58420: URL: https://github.com/apache/spark/pull/58420#issuecomment-5476854342
Thanks for the review, and for running the three arms - your ablation table matches what I measured here. Merged to `master`, `branch-4.x` and `branch-4.3`. On the sequencing you asked about: I merged this one first, and that was a practical call rather than a claim of precedence. It was approved with nothing open, yours has a few small items left. Rebase, answer those, and yours can go straight in. **Neither subsumes the other, and I measured that rather than reasoning it out.** I ran this PR's three query-level tests against `265008fc4e8` with this PR *not* applied, and I ran the shape neither of your tests covers against both heads: | shape | master | this PR | #58335 alone | |---|---|---|---| | identity+years, subset keys, AQE | `ClassCastException` | 0 shuffles | 0 shuffles | | three tables, differing key spaces | `ClassCastException` | clean `STORAGE_PARTITION_JOIN_INCOMPATIBLE_REDUCED_TYPES` | right rows, 0 shuffles | | another child re-shuffled onto the reduced layout | driver dies assembling the key map | 2 shuffles, right rows | 1 shuffle, right rows | | **both sides reduce to a third type** (`days` and `years` both reducing to `Long`) | `ClassCastException` | **right rows, 0 shuffles** | **`ClassCastException`** | On the first three you are strictly better, because you remove the cause where this PR only makes the failure clean. The last row is what this PR is really for. When both sides reduce onto a key space no single transform describes, reading the keys at the types they were built with is the only thing that reaches it, and the expression-side approach cannot express it by construction. Both of us leave the type-preserving half of that shape open, which is SPARK-59121. Measured on both heads: `b12 JOIN b8 JOIN b6` over ids 0..11 returns 4 rows of 12 with `allowCompatibleTransforms` on, zero shuffles, no error. That is the silent misrouting the gate comment already names. **For your rebase.** Two of this PR's query tests will fail, in both cases because your change makes the shape work: - `SPARK-59120: incompatible reduced key types are reported instead of cast` - the error is no longer raised. The second join reduces the third table onto `years` too, and the query returns the right rows with no shuffle. Worth turning into a "works now" pin. The error path itself stays covered by `SPARK-56046: Reducers with different result types`. - `SPARK-59120: another child is not shuffled onto reducer-rewritten keys` - one shuffle instead of two, onto the now type-correct `years(ts)` layout. The third query test and the two `ShuffleSpecSuite` tests stay green. I have not measured the two stacked, so "they compose" is still reasoning on both our parts, and your rebase settles it. -- 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]
