New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
colexec: make unordered distinct streaming-like #57579
Conversation
Benchmarks are here. I think the increase in allocations in some cases is due to the fact that we now can no longer allocate the output batch of large size once and, instead, we have dynamic batch size behavior for the output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colexec/hashtable.go, line 359 at r1 (raw file):
if ht.buildMode != hashTableDistinctBuildMode { colexecerror.InternalError(errors.AssertionFailedf( "hashTable.fullBuild is called in unexpected build mode %d", ht.buildMode,
nit: s/fullBuild/distinctBuild
Previously, when executing an unordered distinct, we would build the whole hash table and consume the input source entirely before emitting any output. This is a suboptimal behavior when the query has a limit - we're likely to reach the limit long time before consuming the whole input source. This commit makes the unordered distinct more streaming-like - it builds the hash table one batch at a time, and whenever some distinct tuples are appended to the hash table, all of them are emitted in the output. Release note (performance improvement): Previously, CockroachDB when performing an unordered DISTINCT operation via the vectorized execution engine would buffer up all tuples from the input which is a suboptimal behavior when the query has a LIMIT clause, and this has now been fixed. This behavior was introduced in 20.1. Note that the old row-by-row engine doesn't have this issue.
b3a567c
to
18b429f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to add a release note for this, and I think it'll be worth it to backport it to 20.2 since it is a regression between using the vec and the row engines.
TFTR!
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto)
Build succeeded: |
Previously, when executing an unordered distinct, we would build the
whole hash table and consume the input source entirely before emitting
any output. This is a suboptimal behavior when the query has a limit -
we're likely to reach the limit long time before consuming the whole
input source.
This commit makes the unordered distinct more streaming-like - it builds
the hash table one batch at a time, and whenever some distinct tuples
are appended to the hash table, all of them are emitted in the output.
Fixes: #57566.
Release note (performance improvement): Previously, CockroachDB when
performing an unordered DISTINCT operation via the vectorized execution
engine would buffer up all tuples from the input which is a suboptimal
behavior when the query has a LIMIT clause, and this has now been fixed.
This behavior was introduced in 20.1. Note that the old row-by-row
engine doesn't have this issue.