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

Improve aggregation error message #2150

Merged
merged 2 commits into from
Aug 23, 2023
Merged

Improve aggregation error message #2150

merged 2 commits into from
Aug 23, 2023

Conversation

PSeitz
Copy link
Contributor

@PSeitz PSeitz commented Aug 18, 2023

Improve aggregation error message by wrapping the deserialization with a
custom struct. This deserialization variant is slower, since we need to
keep the deserialized data around twice with this approach.
For now the valid variants list is manually updated. This could be
replaced with a proc macro.
closes #2143

Improve aggregation error message by wrapping the deserialization with a
custom struct. This deserialization variant is slower, since we need to
keep the deserialized data around twice with this approach.
For now the valid variants list is manually updated. This could be
replaced with a proc macro.
closes #2143
@PSeitz PSeitz requested a review from fulmicoton August 18, 2023 18:09
@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2023

Codecov Report

Merging #2150 (24be683) into main (62ece86) will increase coverage by 0.00%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main    #2150   +/-   ##
=======================================
  Coverage   94.32%   94.33%           
=======================================
  Files         320      320           
  Lines       62041    62054   +13     
=======================================
+ Hits        58522    58539   +17     
+ Misses       3519     3515    -4     
Files Changed Coverage Δ
src/query/boost_query.rs 73.17% <ø> (ø)
src/aggregation/agg_req.rs 94.84% <100.00%> (+0.27%) ⬆️
src/aggregation/agg_tests.rs 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

/// Aggregation request.
///
/// An aggregation is either a bucket or a metric.
pub struct AggregationWrapper {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why pub and why inline the struct?

@fulmicoton
Copy link
Collaborator

@PSeitz I pushed a commit with a less brittle approach. I leave it to you to modify if necessary and merge.

@PSeitz
Copy link
Contributor Author

PSeitz commented Aug 23, 2023

@PSeitz I pushed a commit with a less brittle approach. I leave it to you to modify if necessary and merge.

Nice, I didn't know about try_from in serde

@PSeitz PSeitz merged commit 48d4847 into main Aug 23, 2023
5 checks passed
@PSeitz PSeitz deleted the agg_error branch August 23, 2023 18:52
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.

Fix aggregation deserialization error message.
3 participants