Add support for keyed parameter in range and histgram aggregations #1424

k-yomo · 2022-07-25T18:09:15Z

Resolves #1336

This PR adds support for keyed parameter in range and histogram aggregations in the same way elasticsearch does.

codecov-commenter · 2022-07-25T18:25:59Z

Codecov Report

Merging #1424 (a9b0d1a) into main (931bab8) will increase coverage by 0.01%.
The diff coverage is 99.25%.

@@            Coverage Diff             @@
##             main    #1424      +/-   ##
==========================================
+ Coverage   94.22%   94.23%   +0.01%     
==========================================
  Files         236      236              
  Lines       43775    43898     +123     
==========================================
+ Hits        41245    41367     +122     
- Misses       2530     2531       +1

Impacted Files	Coverage Δ
src/aggregation/agg_req.rs	`95.23% <90.00%> (-0.32%)`	⬇️
src/aggregation/agg_req_with_accessor.rs	`94.32% <100.00%> (-0.04%)`	⬇️
src/aggregation/agg_result.rs	`74.00% <100.00%> (+0.53%)`	⬆️
src/aggregation/bucket/histogram/histogram.rs	`99.61% <100.00%> (+0.01%)`	⬆️
src/aggregation/bucket/range.rs	`96.62% <100.00%> (+0.32%)`	⬆️
src/aggregation/intermediate_agg_result.rs	`98.30% <100.00%> (+0.07%)`	⬆️
src/aggregation/metric/stats.rs	`97.21% <100.00%> (+0.01%)`	⬆️
src/aggregation/mod.rs	`98.87% <100.00%> (+0.09%)`	⬆️
bitpacker/src/bitpacker.rs	`96.03% <0.00%> (-1.00%)`	⬇️

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 931bab8...a9b0d1a. Read the comment docs.

src/aggregation/bucket/histogram/histogram.rs

src/aggregation/bucket/range.rs

src/aggregation/agg_result.rs

PSeitz · 2022-07-26T08:40:11Z

src/aggregation/intermediate_agg_result.rs

+                    .keyed
+                    .is_some();
+                let buckets = if is_keyed {
+                    let mut bucket_map = HashMap::new();


we can use with_capacity here

fixed! 80a1418

PSeitz · 2022-07-26T08:45:07Z

Awesome thank you!
One thing that is missing are custom keys in ranges. Can be done in this PR or we create a separate issue for that.

GET sales/_search
{
  "aggs": {
    "price_ranges": {
      "range": {
        "field": "price",
        "keyed": true,
        "ranges": [
          { "key": "cheap", "to": 100 },
          { "key": "average", "from": 100, "to": 200 },
          { "key": "expensive", "from": 200 }
        ]
      }
    }
  }
}

k-yomo · 2022-07-26T08:48:20Z

@PSeitz
Thanks for the review!! I'll add custom keys support in this PR🙏

k-yomo · 2022-07-26T09:08:13Z

@PSeitz
Oh sorry, I thought custom keys are already there and not working with keyed but it's actually not implemented yet?

If so, it would be great if we could have a separate issue🙏
I would be happy to work on it though!

PSeitz · 2022-07-26T10:03:07Z

@PSeitz Oh sorry, I thought custom keys are already there and not working with keyed but it's actually not implemented yet?

If so, it would be great if we could have a separate issue I would be happy to work on it though!

Okay cool, I created an issue for that here: #1425

PSeitz · 2022-07-26T15:47:12Z

src/aggregation/agg_req.rs

@@ -36,7 +37,8 @@
 //!       "ranges": [
 //!         { "from": 3.0, "to": 7.0 },
 //!         { "from": 7.0, "to": 20.0 }
-//!       ]
+//!       ],
+//!       "keyed": false


this is the default, I think we can just omit it

It didn't pass the tests...
https://github.com/quickwit-oss/tantivy/runs/7517376126?check_suite_focus=true

Ah I see, I removed serde(default) by mistake, let me fix

fixed in 6444516

PSeitz · 2022-07-26T15:58:06Z

src/aggregation/mod.rs

@@ -881,7 +891,8 @@ mod tests {
        { "from": 7.0, "to": 19.0 },
        { "from": 19.0, "to": 20.0 },
        { "from": 20.0 }
-      ]
+      ],
+      "keyed": false


do we have to add this? the default behaviour should be the same

PSeitz · 2022-07-27T09:26:41Z

src/aggregation/mod.rs

@@ -132,6 +132,7 @@
 //!             bucket_agg: BucketAggregationType::Range(RangeAggregation{
 //!                 field: "score".to_string(),
 //!                 ranges: vec![(3f64..7f64).into(), (7f64..20f64).into()],
+//!                 keyed: false,


Suggested change

//! keyed: false,

@PSeitz
Could we do that?
Just removing the field will cause compiler error.

missing field `keyed` in initializer of `RangeAggregation`

ah okay there's no ..Default::default, we can keep it here

PSeitz · 2022-07-27T09:27:02Z

src/aggregation/mod.rs

            },
            "aggs": {
                "bucketsL2": {
                    "histogram": {
                        "field": "score",
-                        "interval":  70.0
+                        "interval":  70.0,
+                        "keyed": false


Suggested change

"keyed": false

Oh sorry missed those, fixed in 9b6b60c

PSeitz · 2022-07-27T09:27:10Z

src/aggregation/mod.rs

@@ -536,7 +541,8 @@ mod tests {
                "bucketsL2": {
                    "histogram": {
                        "field": "score",
-                        "interval":  70.0
+                        "interval":  70.0,
+                        "keyed": false


Suggested change

"keyed": false

k-yomo force-pushed the support-keyed-parameter-in-aggregation branch from 3f623df to 53097ab Compare July 25, 2022 19:13

k-yomo added 2 commits July 26, 2022 04:28

Add support for keyed parameter in range and histgram aggregations

5b56491

Add tests for keyed buckets

d122f2c

k-yomo force-pushed the support-keyed-parameter-in-aggregation branch from 53097ab to d122f2c Compare July 25, 2022 19:28

k-yomo marked this pull request as ready for review July 25, 2022 19:30

PSeitz reviewed Jul 26, 2022

View reviewed changes

src/aggregation/bucket/histogram/histogram.rs Outdated Show resolved Hide resolved

PSeitz reviewed Jul 26, 2022

View reviewed changes

src/aggregation/bucket/range.rs Outdated Show resolved Hide resolved

PSeitz reviewed Jul 26, 2022

View reviewed changes

src/aggregation/agg_result.rs Outdated Show resolved Hide resolved

PSeitz reviewed Jul 26, 2022

View reviewed changes

k-yomo added 4 commits July 26, 2022 18:18

Fix to use bool directory for the keyed parameter

5ab5f07

Use FnvHashMap for keyed bucket entries

80a1418

Fix keyed param type in the comment

2b333ca

Fix aggreagtion examples

a9b0d1a

k-yomo requested a review from PSeitz July 26, 2022 10:17

PSeitz reviewed Jul 26, 2022

View reviewed changes

User serde default for the keyed params

6444516

k-yomo requested a review from PSeitz July 26, 2022 16:35

PSeitz reviewed Jul 27, 2022

View reviewed changes

Remove unnecessary keyed parameter setting

9b6b60c

k-yomo requested a review from PSeitz July 27, 2022 09:58

PSeitz approved these changes Jul 27, 2022

View reviewed changes

PSeitz merged commit da0f78e into quickwit-oss:main Jul 27, 2022

k-yomo deleted the support-keyed-parameter-in-aggregation branch July 27, 2022 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for keyed parameter in range and histgram aggregations #1424

Add support for keyed parameter in range and histgram aggregations #1424

k-yomo commented Jul 25, 2022 •

edited

codecov-commenter commented Jul 25, 2022 •

edited

PSeitz Jul 26, 2022

k-yomo Jul 26, 2022

PSeitz commented Jul 26, 2022

k-yomo commented Jul 26, 2022

k-yomo commented Jul 26, 2022 •

edited

PSeitz commented Jul 26, 2022

PSeitz Jul 26, 2022

k-yomo Jul 26, 2022 •

edited

k-yomo Jul 26, 2022

PSeitz Jul 26, 2022 •

edited

PSeitz Jul 27, 2022

k-yomo Jul 27, 2022

PSeitz Jul 27, 2022

PSeitz Jul 27, 2022

k-yomo Jul 27, 2022

PSeitz Jul 27, 2022

Add support for keyed parameter in range and histgram aggregations #1424

Add support for keyed parameter in range and histgram aggregations #1424

Conversation

k-yomo commented Jul 25, 2022 • edited

codecov-commenter commented Jul 25, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PSeitz commented Jul 26, 2022

k-yomo commented Jul 26, 2022

k-yomo commented Jul 26, 2022 • edited

PSeitz commented Jul 26, 2022

Choose a reason for hiding this comment

k-yomo Jul 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PSeitz Jul 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-yomo commented Jul 25, 2022 •

edited

codecov-commenter commented Jul 25, 2022 •

edited

k-yomo commented Jul 26, 2022 •

edited

k-yomo Jul 26, 2022 •

edited

PSeitz Jul 26, 2022 •

edited