Skip to content

fix: don't re-copy already emitted strings in Utf8View group-by emit - #27360

Open
Kristofferjoha wants to merge 1 commit into
apache:mainfrom
Kristofferjoha:fix-byte-view-take-n-copy
Open

Kristofferjoha wants to merge 1 commit into
apache:mainfrom
Kristofferjoha:fix-byte-view-take-n-copy

Conversation

@Kristofferjoha

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

A streaming GROUP BY with a Utf8View column among several group columns emits small batches that carry far more string data than their rows use. The cause is in ByteViewGroupValueBuilder::take_n. On each partial emit it copies the in-progress string buffer from byte 0 up to the last emitted string, and never trims that buffer. So every emitted batch carries the strings of all earlier batches from the same block.

What changes are included in this PR?

After a partial take from the in-progress buffer, drain the taken bytes from it and shift the offsets of the remaining views. Each emitted batch then holds only its own strings. drain moves the not-yet-emitted bytes to the front of the buffer on each partial emit. In the streaming case that is only a few groups.

Not changed: when the last emitted string is in a completed block, the emitted batch still shares that whole block (up to 2 MB). In the streaming case that should now be rare, since the in-progress buffer only holds groups not yet emitted.

What is the testing strategy for this PR?

New unit test test_byte_view_take_n_drains_in_progress. It takes one row at a time and checks that each emitted array's data buffer holds only its own string, that the in-progress buffer shrinks, and that the remaining and newly appended values are still correct.

I also ran the repro from the issue with the cli, and compared to main:

Memory limit main This PR
-m 200M ResourcesExhausted passes
-m 50M ResourcesExhausted passes
-m 20M ResourcesExhausted passes
-m 10M ResourcesExhausted passes

EXPLAIN ANALYZE of the same query without a memory limit:

Operator main output_bytes This PR output_bytes
AggregateExec (Partial) 42.6 MB 1023.1 KB
RepartitionExec 330.0 MB 5.5 MB

Are there any user-facing changes?

No

@Kristofferjoha

Copy link
Copy Markdown
Contributor Author

run benchmarks

@adriangbot

Copy link
Copy Markdown

@Kristofferjoha

Copy link
Copy Markdown
Contributor Author

@adriangb this fixes the aggregate part of #27323 . Could you run the benchmarks on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants