Add rollup functionality to frequency aggregates #696
Conversation
| counts: &[u64], | ||
| overcounts: &[u64], | ||
| ) { | ||
| self.total_vals = val_count; |
There was a problem hiding this comment.
Should this one assert TEXTOID?
There was a problem hiding this comment.
Also wondering this, unless there's a reason for only checking it in ingest_aggregate_ints()?
There was a problem hiding this comment.
This same function is called by both text aggregates and raw aggregates (the AnyElement version of this). So the OID can really be anything here.
| self.total_vals = val_count; | ||
|
|
||
| for (idx, datum) in values.iter().enumerate() { | ||
| self.entries.push(SpaceSavingEntry { |
There was a problem hiding this comment.
Should these assert entries is empty?
It seems these are just constructor part two; have you considered folding the calls in?
There was a problem hiding this comment.
Asserting entries is empty is probably a good idea. As for constructors, the problem is that there are a couple constructors and which one is used is completely independent of which call we use to fill them. So if I wanted to fold them in I'd have to have a function for every combination of calls.
WireBaron
force-pushed
the
br/freq_rollup
branch
from
a140063
to
a03dfce
Compare
That was my thought as well, that test shouldn't be affected by this change at all. Still, I'll take a closer look and see if we can make the test less flakey. |
|
bors r+ |
696: Add rollup functionality to frequency aggregates r=WireBaron a=WireBaron This change implements adds in rollup functions for frequency aggregates. Note that while the rollup of a set of frequency aggregates will not necessarily be identical to computing a single aggregate over the underlying data (may not have the exact same upper and lower bounds on frequency), the rollup maintains the same invariants and will be able to identify most common elements as long as the frequency is different enough. Fixes #685 Co-authored-by: Brian Rowe <[email protected]>
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
This change adds rollup functions for frequency aggregates.
Note that while the rollup of a set of frequency aggregates will not necessarily be identical to computing a single aggregate over the underlying data (may not have the exact same upper and lower bounds on frequency), the rollup maintains the same invariants and will be able to identify most common elements as long as the frequency is different enough.
Fixes #685