jayzhan211 commented on PR #26116:
URL: https://github.com/apache/datafusion/pull/26116#issuecomment-6381187490
@Rachelint I tried this locally. A few things before it's ready for review:
1. CI fails on
`ungrouped_utf8_view_accumulator_is_never_worse_than_a_pre_allocated_set`.
`ArrowBytesViewMap::with_capacity(n)` with `n > 0` now turns on the progressive
StringView blocks, so `count(DISTINCT)` on `Utf8View` gets them too. Making
them an explicit opt-in fixes it: for example a `with_progressive_blocks()`
builder that `take` and `clear_and_release` keep, called only by
`GroupValuesBytesView`. I have a patch if it helps.
2. The StringView block change is independent of flushing. As its own PR it
would be easier to review and to measure, which also matches what you wrote in
#26124.
3. There are three settings for the same decision (`flush_batch`,
`flush_rows`, `flush_bytes`). On the bot, flushing at the batch size gave
0.89–0.93x total time across runs, while the 262144-row threshold was neutral
(0.995x and 1.013x, with 7–9 queries slower). Could the PR keep only the
batch-size flush? If the byte threshold stays, an empty input batch with a very
small `partial_aggregation_flush_bytes` ends in the internal error "tried to
flush an empty table". Checking that the table has groups before flushing fixes
it.
4. It is slower when a moderate number of keys repeat. ClickBench has few
queries like that, so the bot doesn't show it. Locally (M4, `hits_partitioned`,
default settings plus the batch-size flush, min of warm runs, two runs):
| `SELECT "UserID" % N AS k, COUNT(*), SUM("ResolutionWidth") ... GROUP
BY k` | main | this PR |
|---|---|---|
| N = 10,000 | 125 ms | 156 ms (1.25x) |
| N = 20,000 | 133 ms | 177 ms (1.33x) |
| N = 50,000 | 153 ms | 199 ms (1.30x) |
| N = 100,000 | 188 ms | 191 ms (1.02x) |
| N = 200,000 | 236 ms | 179 ms (0.76x), RSS 1.1 → 0.6 GiB |
| N = 2,000,000 | 313 ms | 224 ms (0.72x), RSS 2.7 → 0.9 GiB |
Below ~100k keys, every flush emits nearly the same keys again, so the
Partial stage stops reducing the input but still pays for the table. #25724 has
a check for this: at each flush it samples about 1024 group hashes, and once a
flush finds more than 20% of an earlier flush's sample again, it stops flushing
for the rest of the stream. Something like that should cover these queries
without losing the wins at 200k and above. I haven't measured it with
batch-size flushes yet, but I'm happy to move it into this PR.
If you're busy, I can pick these up.
--
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]
