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.

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

feat: add support for u64,i64,f64 fields in term aggregation #1883

Merged
merged 4 commits into from
Feb 27, 2023

Conversation

PSeitz
Copy link
Contributor

@PSeitz PSeitz commented Feb 20, 2023

No description provided.

@PSeitz PSeitz force-pushed the term_agg_more_types branch 3 times, most recently from 737bb4d to a04cbbc Compare February 20, 2023 06:43
impl Eq for Key {}
impl std::hash::Hash for Key {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
core::mem::discriminant(self).hash(state);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't work well if someone indexes different values of f64::NAN and then does an aggregation on it.
Do we care about this use case?

Copy link
Collaborator

@fulmicoton fulmicoton Feb 22, 2023

Choose a reason for hiding this comment

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

Let's forbid NaN entirely. If you really want to keep them we can work at least map them upon input to whatever u64_to_f64(0u64) is

Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL about Discriminant

@PSeitz PSeitz requested a review from fulmicoton February 20, 2023 06:54
@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2023

Codecov Report

Merging #1883 (1737d42) into main (e2aa5af) will increase coverage by 0.00%.
The diff coverage is 95.54%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff            @@
##             main    #1883    +/-   ##
========================================
  Coverage   94.64%   94.65%            
========================================
  Files         301      301            
  Lines       54924    55038   +114     
========================================
+ Hits        51985    52095   +110     
- Misses       2939     2943     +4     
Impacted Files Coverage Δ
src/aggregation/mod.rs 98.20% <88.88%> (-0.07%) ⬇️
src/aggregation/bucket/term_agg.rs 96.34% <95.41%> (+0.04%) ⬆️
src/aggregation/intermediate_agg_result.rs 97.74% <100.00%> (+0.05%) ⬆️
src/aggregation/segment_agg_result.rs 96.31% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@PSeitz
Copy link
Contributor Author

PSeitz commented Feb 20, 2023

#1689

@PSeitz PSeitz force-pushed the term_agg_more_types branch from a04cbbc to 1737d42 Compare February 20, 2023 11:40
@PSeitz PSeitz force-pushed the term_agg_more_types branch 2 times, most recently from 40f2b7f to 6c448d3 Compare February 22, 2023 05:41
@PSeitz PSeitz requested a review from fulmicoton February 27, 2023 04:47
@PSeitz PSeitz force-pushed the term_agg_more_types branch from 960c03f to 6ad2d1e Compare February 27, 2023 06:43
@PSeitz PSeitz merged commit e510f69 into main Feb 27, 2023
@PSeitz PSeitz deleted the term_agg_more_types branch February 27, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants