colexec: add support for DISTINCT ordered aggregation #53145
Conversation
|
This change is |
|
Wrapped processor performance on master: Wrapped processor performance with the first commit: Vectorized operator performance: (Note that I did update the benchmark harness to properly recreate the operator chain - with |
yuzefovich
force-pushed
the
distinct-ord-agg
branch
3 times, most recently
from
7bd4098 to
0dbe896
Compare
yuzefovich
force-pushed
the
distinct-ord-agg
branch
from
0dbe896 to
ec0b7d0
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 8 of 8 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colexec/aggregate_funcs.go, line 371 at r2 (raw file):
// seen is a slice of maps used to handle distinct aggregation. A // corresponding entry in the slice is nil if the function doesn't have a // DISTINCT clause. The slice itself will be nil whenever no aggregate
nit: do you mean the map instead of the slice?
pkg/sql/colexec/aggregators_util.go, line 41 at r2 (raw file):
// tuples that pass the criteria - like DISTINCT and/or FILTER will be // aggregated). performAggregation(ctx context.Context, vecs []coldata.Vec, inputLen int, sel []int, bucket *aggBucket, groups []bool)
Add reference to groups in comment.
yuzefovich
force-pushed
the
distinct-ord-agg
branch
2 times, most recently
from
49b87e6 to
c9c0f3a
Compare
There was a problem hiding this comment.
I think it is likely that the vectorized hash table will still show worse performance, and I intend to prototype it today, so I don't think an issue is necessary.
TFTR!
bors r+
Reviewable status:
pkg/sql/colexec/aggregate_funcs.go, line 371 at r2 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
nit: do you mean the map instead of the slice?
No, I did mean the "slice of maps" will be nil.
pkg/sql/colexec/aggregators_util.go, line 41 at r2 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Add reference to
groupsin comment.
Done.
|
Huh, merge conflict. bors r- |
|
Canceled. |
Previously, whenever we needed to clear `seen` map (which tracks the tuples we have already seen for the aggregation group), we would create a new map. However, it turns out that it is actually faster (according to micro-benchmarks) to delete all entries from the old map instead which is what this commit does. Release note: None
This commit adds the support for DISTINCT ordered aggregation by reusing the same code (with minor modification to reset `seen` maps when the new group is encountered) as we have for hash aggregation. Quick benchmarks show about 6-7x improvement when comparing against a wrapped row-by-row processor. Release note (sql change): Vectorized execution now natively supports ordered aggregation with DISTINCT clause.
yuzefovich
force-pushed
the
distinct-ord-agg
branch
from
c9c0f3a to
c5d5760
Compare
|
As I expected, the vectorized hash table is worse (especially on small group sizes it's just terrible) because our hash table is pretty heavy-weight since it has several constant allocations of bors r+ |
|
The fact we do distinct tuple-at-a-time makes me think there are definitely some performance optimizations we could take advantage of. Is it a question of growing those allocations dynamically or something else? I guess the issue is more so that you can record your thoughts if you can think of anything for the future. |
|
It's a matter of hash table allocating many slices (I think 9 of them in this use case) of |
|
The following error was encountered on the bors run: https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_UnitTests/2208105? |
|
I'll take a look, but I think it is more likely that #53169 is the culprit and not this PR. |
|
that's bad isn't it? This means that all current and future bors runs will have a chance to fail? |
|
It'll be random because we randomize |
|
thanks for checking |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
|
I prototyped handling DISTINCT aggregation using more dynamic hash table (on top of #53226), and it turned out that the performance of distinct hash aggregation is better with current encoding strategy, but for the ordered aggregator it is better with the vectorized hash table. I'll open up an issue. |
rowexec: speed up DISTINCT aggregation a bit
Previously, whenever we needed to clear
seenmap (which tracks thetuples we have already seen for the aggregation group), we would create
a new map. However, it turns out that it is actually faster (according
to micro-benchmarks) to delete all entries from the old map instead
which is what this commit does.
Release note: None
colexec: add support for DISTINCT ordered aggregation
This commit adds the support for DISTINCT ordered aggregation by
reusing the same code (with minor modification to reset
seenmaps whenthe new group is encountered) as we have for hash aggregation. Quick
benchmarks show about 6-7x improvement when comparing against a wrapped
row-by-row processor.
Closes: #39242.
Release note (sql change): Vectorized execution now natively supports
ordered aggregation with DISTINCT clause.