andygrove commented on PR #6095:
URL:
https://github.com/apache/datafusion-comet/pull/6095#issuecomment-5778184784
The partitioning bench is in tree now, and running it past the flush changes
the conclusion enough
that I want to flag it before anyone quotes the placement numbers.
It lives in `benches/shuffle_writer.rs` as a `shuffle_partitioning` group,
with the same fixture
shape you used — 8 batches of 8192 rows into 50 partitions, so
`AUTO_GROUP_ROWS` is 163. Two phases
per arm: `place` is your `partitioning_only`, and `place+gather` adds
`shuffle_write` through a
writer that drops its batches, so it covers `RunIterator` without dragging
in the IPC encode or the
disk. Rather than widen the exports I put one opaque handle behind
`#[doc(hidden)] bench_support`;
the repartitioner and the `PartitionWriter` trait stay crate-private. I also
took your point about
the fixture and gave the nested leaves a per-row fill, which is why my
nested `HashAll` place is
18.25 ms where yours was 12.72 — murmur3 now has varying strings to walk.
M3 Max, intervals under 1% almost throughout:
| schema | phase | `HashAll` | `HashAll{1}` | `RowGroups(auto)` |
`RowGroups(8192)` |
| ------ | -------------- | --------- | ------------ | ----------------- |
----------------- |
| plain | `place` | 857.3 µs | 239.2 µs | 7.91 µs |
3.46 µs |
| plain | `place+gather` | 1.453 ms | 831.4 µs | 178.9 µs |
4.92 µs |
| nested | `place` | 18.25 ms | 764.1 µs | 146.4 µs |
142.9 µs |
| nested | `place+gather` | 36.83 ms | 18.33 ms | 8.274 ms |
180.9 µs |
Your suspicion that the flush is where more of the win is turns out to be
half right, and the
interesting half is the other one. Subtracting the rows, the nested gather
is 18.58 ms for
`HashAll` against 8.13 ms for `RowGroups(auto)` — only 2.3x, because both
still copy all 65536 rows
and the run shape only changes the chunk size they copy in. Placement is
125x cheaper but the
gather is 2.3x, so including it the honest nested figure is 4.5x, not 125x.
The run representation
is still clearly worth having on its own, as you said, just by less than the
placement-only number
suggests.
What I did not expect is `RowGroups(8192)`. A group as long as a batch makes
every run cover a whole
buffered batch, `RunIterator` passes it straight through, and the gather
drops to 38 µs — 204x
rather than 4.5x. So `AUTO_GROUP_ROWS` is buying its imbalance bound at
about a 46x gather cost on
this shape. That is a much worse trade than I assumed when I picked
`batch_size / num_partitions`,
and I am now unsure the default is right: bounding skew at 163 rows per task
is worth something,
but I doubt it is worth that. Do you have a view? The obvious middle is to
default group size to
the batch size and let a config dial it down, on the grounds that a
positional strategy already
spreads far better than the hash it replaces even at batch granularity.
One caveat on my own numbers, since I gave you a hard time about none of us
having a quiet host:
this is a laptop, and the nested `place+gather` arms allocate around 55 MB
an iteration. I would
not read the last significant figure.
--
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]
