Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 8 commits into from
Jul 27, 2022
Merged

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

merged 8 commits into from
Jul 27, 2022

Conversation

k-yomo
Copy link
Contributor

@k-yomo k-yomo commented Jul 25, 2022 •
edited
Loading

Resolves #1336

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

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2022 •
edited
Loading

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.

@k-yomo k-yomo force-pushed the support-keyed-parameter-in-aggregation branch from 3f623df to 53097ab Compare July 25, 2022 19:13
@k-yomo k-yomo force-pushed the support-keyed-parameter-in-aggregation branch from 53097ab to d122f2c Compare July 25, 2022 19:28
@k-yomo k-yomo marked this pull request as ready for review July 25, 2022 19:30
PSeitz
PSeitz reviewed Jul 26, 2022
View reviewed changes
src/aggregation/intermediate_agg_result.rs Outdated
.keyed
.is_some();
let buckets = if is_keyed {
let mut bucket_map = HashMap::new();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use with_capacity here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed! 80a1418

@PSeitz
Copy link
Contributor

PSeitz commented Jul 26, 2022

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
Copy link
Contributor Author

k-yomo commented Jul 26, 2022

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

@k-yomo
Copy link
Contributor Author

k-yomo commented Jul 26, 2022 •
edited
Loading

@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
Copy link
Contributor

PSeitz commented Jul 26, 2022

@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

@k-yomo k-yomo requested a review from PSeitz July 26, 2022 10:17
PSeitz
PSeitz reviewed Jul 26, 2022
View reviewed changes
src/aggregation/agg_req.rs Outdated
@@ -36,7 +37,8 @@
//! "ranges": [
//! { "from": 3.0, "to": 7.0 },
//! { "from": 7.0, "to": 20.0 }
//! ]
//! ],
//! "keyed": false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@k-yomo k-yomo Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 6444516

PSeitz
PSeitz reviewed Jul 26, 2022
View reviewed changes
src/aggregation/mod.rs Outdated
@@ -881,7 +891,8 @@ mod tests {
{ "from": 7.0, "to": 19.0 },
{ "from": 19.0, "to": 20.0 },
{ "from": 20.0 }
]
],
"keyed": false
Copy link
Contributor

@PSeitz PSeitz Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@k-yomo k-yomo requested a review from PSeitz July 26, 2022 16:35
PSeitz
PSeitz reviewed Jul 27, 2022
View reviewed changes
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! keyed: false,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

missing field `keyed` in initializer of `RangeAggregation`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

PSeitz
PSeitz reviewed Jul 27, 2022
View reviewed changes
src/aggregation/mod.rs Outdated
},
"aggs": {
"bucketsL2": {
"histogram": {
"field": "score",
"interval": 70.0
"interval": 70.0,
"keyed": false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"keyed": false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry missed those, fixed in 9b6b60c

PSeitz
PSeitz reviewed Jul 27, 2022
View reviewed changes
src/aggregation/mod.rs Outdated
@@ -536,7 +541,8 @@ mod tests {
"bucketsL2": {
"histogram": {
"field": "score",
"interval": 70.0
"interval": 70.0,
"keyed": false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"keyed": false

@k-yomo k-yomo requested a review from PSeitz July 27, 2022 09:58
@PSeitz PSeitz merged commit da0f78e into quickwit-oss:main Jul 27, 2022
@k-yomo k-yomo deleted the support-keyed-parameter-in-aggregation branch July 27, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PSeitz PSeitz PSeitz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Aggregation: Support keyed parameter
3 participants
@k-yomo @codecov-commenter @PSeitz