Repository navigation
fix: don't re-copy already emitted strings in Utf8View group-by emit - #27360
Open
Kristofferjoha wants to merge 1 commit into
Open
Kristofferjoha wants to merge 1 commit into
Kristofferjoha wants to merge 1 commit into
Conversation
Contributor
Author
|
run benchmarks |
|
Hi @Kristofferjoha, thanks for the request (#27360 (comment)). Only whitelisted users can trigger benchmarks. Allowed users: 2010YOUY01, Dandandan, Fokko, HippoBaro, Jefffrey, Omega359, Rachelint, Rich-T-kid, adriangb, alamb, asubiotto, avantgardnerio, brunal, buraksenn, cetra3, codephage2020, coderfender, comphead, erenavsarogullari, etseidl, friendlymatthew, gabotechs, geoffreyclaude, grtlr, haohuaijin, jayzhan211, jonathanc-n, kevinjqliu, klion26, kosiew, kumarUjjawal, kunalsinghdadhwal, liamzwbao, mbutrovich, mkleen, mzabaluev, neilconway, rluvaton, sdf-jkl, timsaucer, xudong963, zhuqi-lucas. File an issue against this benchmark runner |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
drainmoves 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:
main-m 200M-m 50M-m 20M-m 10MEXPLAIN ANALYZEof the same query without a memory limit:mainoutput_bytesAggregateExec(Partial)RepartitionExecAre there any user-facing changes?
No