LuciferYang commented on PR #58419: URL: https://github.com/apache/spark/pull/58419#issuecomment-5812554267
Rewrote the testing section of the description. It said ten cases in `UnionCodegenSuite` and three in `SparkSessionExtensionSuite` where the net diff has fifteen and eight, and it left out the `AdaptiveQueryExecSuite` case that pins the AQE rule order. It now says what each case pins, grouped by suite, with no counts to go stale. Catching up on the eight threads from the last two rounds that I never answered in place. All of them are in the pinned revision: - [`supportCodegenFailureReason` memoized before a stamp](https://github.com/apache/spark/pull/58419#discussion_r4059229237): it is a `def` again, only the child-topology terms are memoized, and `metrics` registers `numOutputRows` while a node is unstamped. - [AQE-enabled codegen-conf coverage](https://github.com/apache/spark/pull/58419#discussion_r4059229241): `a union fused under AQE keeps both codegen decisions through execution` covers the stamped node, and the extension case added today covers a fresh union at the late AQE barrier. - [the `wholeStage.union.enabled` description](https://github.com/apache/spark/pull/58419#discussion_r4059229244): it now describes an eligible union on its non-partitioning-aware path taking part, with the other gates and the `InputAdapter` boundary stated. - [subjectless sentences in the tag lifecycle](https://github.com/apache/spark/pull/58419#discussion_r4059229246): rewritten as complete sentences that name the rule doing the write and the value written. - [the window between two injected prep rules](https://github.com/apache/spark/pull/58419#discussion_r4068257893): `SnapshotUnionPreparationConf.before` lists a snapshot pass ahead of each injected prep rule, and of each post planner strategy rule. - [comma before the subject](https://github.com/apache/spark/pull/58419#discussion_r4068257907): fixed. - [where the value is captured](https://github.com/apache/spark/pull/58419#discussion_r4068257913): both codegen conf descriptions say it is read once per physical preparation. - [ungrammatical helper comment](https://github.com/apache/spark/pull/58419#discussion_r4068257917): now reads `the node whose tags and metrics the callers here inspect`. @dongjoon-hyun on 4.2: `spark.sql.codegen.wholeStage.union.enabled` is `createWithDefault(false)` on `branch-4.2`, so the crash half is not reachable there by default, while `spark.sql.unionOutputPartitioning` has defaulted to true since 4.1, so the wrong-result half is. That makes me doubt a trimmed variant is worth it. Dropping the two codegen fields from the snapshot would not shrink the blast radius: the three non-result behavior changes listed in the description all follow from taking the partitioning decision during preparation, which any fix for the wrong result has to do. What it would drop is the half with no footprint on 4.2, and the half that makes the codegen flag safe for anyone who turns it on there. So I lean towards taking this as it stands to 4.2, as its own PR against the branch. If you see a narrower shape that still closes the wrong result, I would rather hear it before I open that one. -- 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]
