colexec: refactor hash aggregator #51337
Merged
Conversation
This file contains 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
|
This change is |
|
The benchmarks of the first commit are here and of the second commit are here. The benchmarks of the third commit are here and here. Hash aggregation improves, but for some reason ordered aggregation has unintuitive to me changes - it speeds up when nulls are not present and slows down when nulls are present. Still, I think the third commit is worth it. And the benchmarks of the whole PR: here and here. The highlight: |
yuzefovich
force-pushed
the
hash-agg
branch
7 times, most recently
from
b0a3c17 to
6f5d819
Compare
|
Friendly ping. |
|
I think the performance hit that we see in cases when the group sizes are big can be alleviated (or even entirely eliminated) if we add rehashing behavior to the hash table and start out with small number of buckets (#52257). |
yuzefovich
force-pushed
the
hash-agg
branch
2 times, most recently
from
c28de12 to
c383614
Compare
This commit improves some of the resetting behavior in the hash table (now we copy over the whole slices rather than resetting one element at a time) as well as introduces nicer shorter-named variables for some slice accesses. It also moves one non-templated method outside of the template into a regular file. Release note: None
This commit refactors the hash aggregator to use the vectorized hash table instead of Go's map and tightly couples the hash table with the actual aggregation. The hash table is used in a somewhat similar mode to how unordered distinct uses it with some crucial differences. Now the algorithm for online aggregation is as follows: 1. read batch from the input 2. group all tuples from the batch into "equality chains" (this is done by "probing" the batch against itself - similar to unordered distinct - but instead of "head" tuples into the hash table, we populate special "equality" selection vectors as well as a separate selection vector that contains heads of the equality chains) 3. probe the heads of the equality chains against already existing buckets in the hash table 4. if there are any matches, it means that all tuples in the corresponding equality chains belong to existing groups and are aggregated. 5. all unmatched equality chains form new groups, so we create a separate bucket for each and aggregate the tuples into it. The crucial observation here is that we're maintaining a 1-to-1 mapping between the "heads" of the aggregation groups that are stored in the hash table and the "bucket" of aggregate function in `buckets` slice. This fundamental change to the hash aggregator's algorithm shows 4-5x speedups when the group sizes are small and has tolerable 30-40% hit when the group sizes are big. Such tradeoff is acceptable since the absolute speed in the latter case is still very high. Release note: None
This commit does the following: 1. changes the signature of `Flush` method to take in `outputIdx` argument which is used by the hash aggregate functions to know where to write its output (this argument is ignored by the ordered aggregate functions). This allows us to remove one `int` from the hash aggregate functions which can be noticeable in case of many groups. 2. changes the signature of `Compute` method to take in "disassembled" batch (separate vectors, input length, and the selection vector). This allows us to not perform the copies of the equality chains into the selection vector of the batch in the hash aggregator. 3. extracts base structs that implement the common functionality of aggregate functions. 4. hashAggregatorAllocSize has been retuned and is now 128 instead of previous 64. Note that I prototyped introduction of `aggregateFuncBase` interface which would be implemented by `orderedAggregateFuncBase` and `hashAggregateFuncBase` structs for the step 3 above, but it showed worse performance (slower speed, slightly more allocations), so that prototype was discarded. It also moves a couple of cancel checks outside of the for loop as well as cleans up a few logic test files. Release note: None
jordanlewis
approved these changes
Aug 6, 2020
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 7 of 7 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @yuzefovich)
pkg/sql/colexec/concat_agg_tmpl.go, line 139 at r3 (raw file):
type concat_AGGKINDAgg struct { // {{if eq "_AGGKIND" "Ordered"}} orderedAggregateFuncBase
I really wish not every aggregate had to know about its context... but I guess you didn't introduce that here, either
yuzefovich
commented
Aug 6, 2020
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/colexec/concat_agg_tmpl.go, line 139 at r3 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I really wish not every aggregate had to know about its context... but I guess you didn't introduce that here, either
Yeah, I don't think this addition makes anything worse. Maybe this instantiation could be moved into a templated constructor method, but that can wait.
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Aug 12, 2020
52174: colexec: add default aggregate function r=yuzefovich a=yuzefovich Depends on #51337. Depends on #52315. **colexec: add default aggregate function** This commit introduces "default" aggregate function which is an adapter from `tree.AggregateFunc` to `colexec.aggregateFunc`. It works as follows: - the aggregator (either hash or ordered) is responsible for converting all necessary vectors to `tree.Datum` columns before calling `Compute` (this allows us to share the conversion between multiple functions if they happened to take in the same columns and between multiple groups in case of the hash aggregator) - the default aggregate function populates "arguments" to be passed into the wrapped `tree.AggregateFunc` and adds them - when the new group is encountered, the result so far is flushed and the wrapped `tree.AggregateFunc` is reset. One detail is that these wrapped `tree.AggregateFunc`s need to be closed, and currently that responsibility lies with the alloc object that is creating them. In the future, we might want to shift the responsibility to the aggregators. Addresses: #43561. Release note: None **colexec: clean up hash aggregate functions** Hash aggregate function always have non-nil `sel`, and this commit removes the code generation for nil `sel` case (meaning it removes the dead code). It also templates out nulls vs no-nulls cases in `bool_and` and `bool_or` aggregates. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
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.
colexec: minor cleanup of the hash table
This commit improves some of the resetting behavior in the hash table
(now we copy over the whole slices rather than resetting one element at
a time) as well as introduces nicer shorter-named variables for some
slice accesses. It also moves one non-templated method outside of the
template into a regular file.
Release note: None
colexec: refactor hash aggregator
This commit refactors the hash aggregator to use the vectorized hash
table instead of Go's map and tightly couples the hash table with the
actual aggregation. The hash table is used in a somewhat similar mode to
how unordered distinct uses it with some crucial differences. Now the
algorithm for online aggregation is as follows:
by "probing" the batch against itself - similar to unordered distinct
"equality" selection vectors as well as a separate selection vector that
contains heads of the equality chains)
buckets in the hash table
corresponding equality chains belong to existing groups and are
aggregated.
a separate bucket for each and aggregate the tuples into it.
The crucial observation here is that we're maintaining a 1-to-1 mapping
between the "heads" of the aggregation groups that are stored in the
hash table and the "bucket" of aggregate function in
bucketsslice.This fundamental change to the hash aggregator's algorithm shows 4-5x
speedups when the group sizes are small and has tolerable 30-40% hit
when the group sizes are big. Such tradeoff is acceptable since the
absolute speed in the latter case is still very high.
Release note: None
colexec: clean up aggregate functions
This commit does the following:
Flushmethod to take inoutputIdxargument which is used by the hash aggregate functions to know where to
write its output (this argument is ignored by the ordered aggregate
functions). This allows us to remove one
intfrom the hash aggregatefunctions which can be noticeable in case of many groups.
Computemethod to take in "disassembled"batch (separate vectors, input length, and the selection vector). This
allows us to not perform the copies of the equality chains into the
selection vector of the batch in the hash aggregator.
aggregate functions.
previous 64.
Note that I prototyped introduction of
aggregateFuncBaseinterfacewhich would be implemented by
orderedAggregateFuncBaseandhashAggregateFuncBasestructs for the step 3 above, but it showedworse performance (slower speed, slightly more allocations), so that
prototype was discarded.
It also moves a couple of cancel checks outside of the for loop as well
as cleans up a few logic test files.
Release note: None