add Histogram aggregation #1306

PSeitz · 2022-03-09T10:21:33Z

No description provided.

codecov-commenter · 2022-03-09T12:08:57Z

Codecov Report

Merging #1306 (6e357c8) into main (4b62f79) will increase coverage by 0.15%.
The diff coverage is 98.86%.

❗ Current head 6e357c8 differs from pull request most recent head f619658. Consider uploading reports for the commit f619658 to get more accurate results

@@            Coverage Diff             @@
##             main    #1306      +/-   ##
==========================================
+ Coverage   94.08%   94.23%   +0.15%     
==========================================
  Files         230      231       +1     
  Lines       39191    40758    +1567     
==========================================
+ Hits        36872    38409    +1537     
- Misses       2319     2349      +30

Impacted Files	Coverage Δ
src/aggregation/bucket/range.rs	`98.37% <ø> (ø)`
src/aggregation/agg_req.rs	`96.22% <82.14%> (-3.02%)`	⬇️
src/aggregation/intermediate_agg_result.rs	`97.91% <96.91%> (-0.97%)`	⬇️
src/aggregation/bucket/histogram/histogram.rs	`99.29% <99.29%> (ø)`
src/aggregation/agg_req_with_accessor.rs	`97.82% <100.00%> (+0.02%)`	⬆️
src/aggregation/agg_result.rs	`100.00% <100.00%> (ø)`
src/aggregation/collector.rs	`100.00% <100.00%> (+4.16%)`	⬆️
src/aggregation/metric/average.rs	`86.20% <100.00%> (+1.72%)`	⬆️
src/aggregation/metric/stats.rs	`98.31% <100.00%> (+0.75%)`	⬆️
src/aggregation/mod.rs	`98.53% <100.00%> (+0.55%)`	⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b62f79...f619658. Read the comment docs.

src/aggregation/agg_result.rs

src/aggregation/bucket/histogram/histogram.rs

fulmicoton · 2022-03-14T05:21:31Z

src/aggregation/bucket/histogram/histogram.rs

+            .count();
+        let num_buckets = self.buckets.len();
+
+        let buckets = self


I understand the saturating_sub tries to address the edge case, but that's a cryptic way to write code.

Buckets is a Vec... There is no need for magic tricks.
For instance, you could compute skip_end first, working on the slice [skip_start..].

You could also, compute skip_end first, and thank skip heading element and collect on the same iterator.

since you solved the riddle I can remove it ;)
I just realized we can simply filter all empty buckets.

Edit: I started with the slice, but here we want to get the elements by value to avoid clones

src/aggregation/segment_agg_result.rs

src/aggregation/mod.rs

src/aggregation/intermediate_agg_result.rs

fulmicoton · 2022-03-14T05:36:25Z

src/aggregation/mod.rs

@@ -239,6 +248,10 @@ pub enum Key {
    F64(f64),
 }

+trait MergeFruits {


Is this trait useful? I'm not sure I understand its point.

Ah it is used to share code in the merge_maps thingy?

Why is it not declared in intermediate_agg_result then, close to that function then?

yes, so multiple buckets can share the merge code on collections

I moved it to intermediate_agg_result

fulmicoton

see inline comments.

fulmicoton · 2022-03-14T05:49:07Z

src/aggregation/segment_agg_result.rs

+pub(crate) struct SegmentHistogramBucketEntry {
+    pub key: f64,
+    pub doc_count: u64,
+    pub sub_aggregation: Option<SegmentAggregationResultsCollector>,


I wonder if we should also keep sub_aggregation as an external Vec<Option<SegmentAggregationResultsCollector>>.

yes, that's something I wanted to test as well, but I would probably go for Option<Vec<SegmentAggregationResultsCollector>>

Yep, it's much faster like that, 2x in some cases

src/aggregation/agg_result.rs

src/aggregation/bucket/histogram/mod.rs

src/aggregation/mod.rs

fulmicoton

See comment inline.

src/aggregation/agg_result.rs

fulmicoton

same file same comment. the code is needlessly complicated.

fulmicoton · 2022-03-18T01:57:23Z

src/aggregation/agg_result.rs

-                .collect(),
-        )
+impl AggregationResults {
+    /// Convert and intermediate result and its aggregation request to the final result


very helpful

src/aggregation/agg_result.rs

src/aggregation/bucket/histogram/histogram.rs

Co-authored-by: Paul Masurel <paul@quickwit.io>

PSeitz force-pushed the histogram branch 5 times, most recently from 4cb8932 to a88e260 Compare March 11, 2022 13:14

Add Histogram aggregation

226f577

PSeitz force-pushed the histogram branch from a88e260 to 226f577 Compare March 11, 2022 13:52