Use standard bit set impl in cardinality #61816
Conversation
This replaces a specialized bit set implementation used in cardinality with our standard `BitArray` which works exactly the same way. Its also tracked by `BigArrays` which is great!
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
elasticmachine
added
the
Team:Analytics
|
run elasticsearch-ci/2 |
There was a problem hiding this comment.
I like the change but I am not happy with the ceremony of checking the range on the BucketOrds.
I had a look into the usage of BitArray and it seems we have a similar ceremony in TopMetricsAggregator. The situation is worst in BucketedSort and ParentJoinAggregator where we are doing a blind cast.
On the other hand there are other usages like in LongValuesSource where working on int make sense. I wonder if we should introduce a BigBitArray that works on long and avoid this ceremony?
We can do that in a follow up PR.
|
I was thinking about that! I believe all of the usages would prefer we
support longs. At least, all of the important usages. I'll have a look.
…On Wed, Sep 2, 2020, 03:17 Ignacio Vera ***@***.***> wrote:
***@***.**** commented on this pull request.
I like the change but I am not happy with the ceremony of checking the
range on the BucketOrds.
I had a look into the usage of BitArray and it seems we have a similar
ceremony in TopMetricsAggregator. The situation is worst in BucketedSort
and ParentJoinAggregator where we are doing a blind cast.
On the other hand there are other usages like in LongValuesSource where
working on int make sense. I wonder if we should introduce a BigBitArray
that works on long and avoid this ceremony?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#61816 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABUXIVUDW4PJA24TRG6UALSDXWP3ANCNFSM4QSDQCSA>
.
|
|
@iverase I've updated this one now that we support longs. We may not need it at all given what we talked about this morning. But if you think we do I'd be happy to merge it. |
iverase
left a comment
iverase
left a comment
There was a problem hiding this comment.
We might still need it, lets merge it from now as it is an improvement of what we have now.
LGTM
|
Thanks @iverase ! |
This replaces a specialized bit set implementation used in cardinality with our standard `BitArray` which works exactly the same way. Its also tracked by `BigArrays` which is great!
4 participants
This replaces a specialized bit set implementation used in cardinality
with our standard
BitArraywhich works exactly the same way. Its alsotracked by
BigArrayswhich is great!