fix postcard compatibility for top_hits, add postcard test #2346
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fulmicoton
reviewed
Apr 8, 2024
| @@ -92,8 +96,9 @@ pub struct TopHitsVecEntry { | |||
|
|
|||
| /// Search results, for queries that include field retrieval requests | |||
| /// (`docvalue_fields`). | |||
| #[serde(flatten)] | |||
| pub search_results: FieldRetrivalResult, | |||
There was a problem hiding this comment.
this should have been FieldRetrievalResult too.
Is it still used? Can we remove or rename it?
There was a problem hiding this comment.
It can't be FieldRetrievalResult, since postcard does not support flatten
fulmicoton
reviewed
Apr 8, 2024
fulmicoton
reviewed
Apr 8, 2024
fulmicoton
reviewed
Apr 8, 2024
fulmicoton
reviewed
Apr 8, 2024
src/aggregation/metric/top_hits.rs
Outdated
| @@ -344,7 +366,7 @@ impl PartialEq for ComparableDocFeature { | |||
| impl Eq for ComparableDocFeature {} | |||
|
|
|||
| #[derive(Clone, Serialize, Deserialize, Debug)] | |||
| struct ComparableDocFeatures(Vec<ComparableDocFeature>, FieldRetrivalResult); | |||
| struct ComparableDocFeatures(Vec<ComparableDocFeature>, IntermediateFieldRetrivalResult); | |||
There was a problem hiding this comment.
Can we fix the terrible struct name and remove the 2 positional arguments and use a regular struct?
There was a problem hiding this comment.
Yes, I did also do a pass on all the names and some comments
fulmicoton
reviewed
Apr 8, 2024
PSeitz
force-pushed
the
fix_top_hits
branch
3 times, most recently
from
38df0c0 to
00752da
Compare
fulmicoton
reviewed
Apr 9, 2024
fulmicoton
reviewed
Apr 9, 2024
fulmicoton
reviewed
Apr 9, 2024
fulmicoton
approved these changes
Apr 9, 2024
fulmicoton
requested changes
Apr 9, 2024
fulmicoton
approved these changes
Apr 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes quickwit-oss/quickwit#4840
Fix intermediate result postcard serialization by removing flatten
Fix intermediate result postcard deserialization by replacing
OwnedValue(no self-describing formats)Delay doc value fetchting to end of segment collection, closes #2347
Fix naming and struct organization