andygrove commented on PR #6095: URL: https://github.com/apache/datafusion-comet/pull/6095#issuecomment-5783438459
Thanks — all four are addressed, in two commits: the scrambled start, and the wording corrections. I'm keeping the auto default, on your argument. The benchmark runs a single map task, so the 204x it reports is the per-task gather cost with the composition problem invisible, and that is the right reason to discount it rather than to build the default around it. One correction to your table, though, because the scrambled start moves it. Every empty-reducer count in it was a consequence of adjacent starts, and they all go to zero once the start comes from `positionalStartPartition`. Same formula and same shapes, starts scrambled: | map tasks | rows per task | partitions | `groupRows` | min per reducer | max per reducer | empty | max/min | | --------- | ------------- | ---------- | ----------- | --------------- | --------------- | ----- | ------- | | 50 | 1,000,000 | 200 | 8192 | 188,416 | 294,912 | 0 | 1.57x | | 50 | 1,000,000 | 200 | 64 (auto) | 249,664 | 250,304 | 0 | 1.00x | | 10 | 500,000 | 200 | 8192 | 8,192 | 49,152 | 0 | 6.00x | | 10 | 500,000 | 200 | 64 (auto) | 24,960 | 25,152 | 0 | 1.01x | So the case against a batch-sized group is no longer "28 idle reducers" but "1.57x on that shape, 6x on the smaller one" — weaker than the table read, and still enough. A `repartition(200)` that hands one reducer six times another's rows is not a stage anyone wants, and the auto default gets to 1.00x for a gather cost that is still 4.5x better than `HashAll` on the nested schema. The large group stays available as an explicit setting for anyone who wants the zero-copy path and knows their task emits enough groups to afford it, and the config doc now says what it gives up. `.ai/skills/review-comet-shuffle-pr/SKILL.md` is updated. The old item said round robin is hash-based on purpose and that a "true" round robin breaks retry determinism, full stop. It now separates the two: `HashAll` stays the default and the reason for it is unchanged, and positional placement gets its own item naming both gates, the row-versus-batch ordinal distinction, and the decorrelated-start requirement, so a reviewer following it checks whether a PR widens a gate rather than flagging the path itself. -- 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]
