ulysses-you commented on PR #57742: URL: https://github.com/apache/spark/pull/57742#issuecomment-5216106706
Thanks for the thorough review, @peter-toth — the eligibility gate and the two path items were things I would not have found on my own. Fixed in 87937dbd6d0: **1 (blocking, ordering).** Aligned the interpreted path to the generated one; details on that [thread](https://github.com/apache/spark/pull/57742#discussion_r3722889760). Added a regression test that walks codegen x two-level-map x spill and asserts one result set — confirmed it fails on the previous code with the two divergent rows. **3 (blocking, tests that never bypass).** Confirmed. The nullable case was 400 rows over 301 keys (ratio 1.33) and the GROUPING SETS case included a `()` set; both now have enough distinct keys to bypass in all eight cells. ROLLUP and CUBE are a different story. I first wrote them off as structurally unable to bypass, but that is wrong — the ratio for `d` distinct rollup columns is `(d+1)N / (dN+1)`, which tends to 1 as `d` grows, and measuring it, a five-column rollup does bypass: ``` ROLLUP d=2 ratio=1.500 bypassed=0 ROLLUP d=3 ratio=1.333 bypassed=0 ROLLUP d=5 ratio=1.200 bypassed=1184 ROLLUP d=20 ratio=1.050 bypassed=4184 ``` At the two columns these tests use they decline, which is worth covering on its own, so I documented that rather than reshaping them. (The turn is also earlier than the algebra predicts — at `d=5` the global ratio is still 1.2 — because the first check lands before the expansion has covered every grouping set. That is your item 2 in miniature.) One correction: the #28804 argument does not rest only on those three. `pass-through fires for high-cardinality input below an Expand` was written for it and does bypass (1192 rows). Your point still improved things — three more tests now reach the feature. **8 (eligibility comment).** Confirmed: `planAggregateWithOneDistinct`'s `partialDistinctAggregate` calls `createAggregate` without `requiredChildDistributionExpressions` while sitting after a shuffle. The comment now says what actually carries the gate — the mode check — and names that counterexample. **9 (streaming).** Confirmed and excluded, matching what `bypassPartialAggregation` does. **11 (metric).** Registered conditionally, following `UnionExec`. Still open: **2 (unrepresentative prefix).** Real, and I do not have a fix I am confident in. The first check is a fixed row count into an input of unknown size, and the decision never reverses. Interested in your view on which lever is right — a check point that scales with what the task has seen, or letting a later check undo the bypass. **6 (cumulative ratio).** Confirmed: the counter only advances, so a late turn is invisible unless a spill resets the epoch. Two options — make the docs match, or make each window judge its own rows — and since the second one trades directly against item 2, I would rather settle 2 first. **7 (`aggTime`).** Confirmed: `doExecute` times only the iterator constructor, and pass-through consumes most of the input from `next()`. The honest fix times `processInputs` and `nextPassThroughOutput` instead, which is a metric change beyond this PR's subject; happy to do it here if you prefer. **4 and 5.** Both confirmed — the result files still say "(Tier 1)" and were measured at `minCompaction = 1.1`. Regenerating them and rewriting the description next, including the `needStopCheck`/`needCopyResult` overrides you flagged. -- 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]
