Add support for keyed parameter in range and histgram aggregations by k-yomo · Pull Request #1424 · quickwit-oss/tantivy

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.

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

+                    .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

 //!         { "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

        { "from": 20.0 }
-      ]
+      ],
+      "keyed": false


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

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

 //!             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

                        "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

                        "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

Comment thread src/aggregation/bucket/histogram/histogram.rs Outdated

PSeitz reviewed Jul 26, 2022

View reviewed changes

Comment thread src/aggregation/bucket/range.rs Outdated

PSeitz reviewed Jul 26, 2022

View reviewed changes

Comment thread src/aggregation/agg_result.rs Outdated

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

Uh oh!

Uh oh!

Conversation

k-yomo commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PSeitz commented Jul 26, 2022

Uh oh!

k-yomo commented Jul 26, 2022

Uh oh!

k-yomo commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PSeitz commented Jul 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k-yomo Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PSeitz Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

k-yomo commented Jul 25, 2022 •

edited

Loading

codecov-commenter commented Jul 25, 2022 •

edited

Loading

k-yomo commented Jul 26, 2022 •

edited

Loading

k-yomo Jul 26, 2022 •

edited

Loading

PSeitz Jul 26, 2022 •

edited

Loading