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

chore!:drop JSON support on intermediate agg result #1992

Merged
merged 3 commits into from
Apr 26, 2023
Merged

Conversation

PSeitz
Copy link
Contributor

@PSeitz PSeitz commented Apr 13, 2023

JSON support is flaky anyway due it's lack on f64::INF etc. handling

add support for other formats by removing skip_serialize and untagged

In conjunction with: quickwit-oss/quickwit#3170

@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023

Codecov Report

Merging #1992 (e4af61c) into main (80df1d9) will decrease coverage by 0.03%.
The diff coverage is 96.77%.

📣 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    #1992      +/-   ##
==========================================
- Coverage   94.35%   94.33%   -0.03%     
==========================================
  Files         320      320              
  Lines       59064    59024      -40     
==========================================
- Hits        55729    55679      -50     
- Misses       3335     3345      +10     
Impacted Files Coverage Δ
src/aggregation/agg_tests.rs 100.00% <ø> (ø)
src/aggregation/mod.rs 93.90% <ø> (-2.78%) ⬇️
src/aggregation/intermediate_agg_result.rs 94.83% <96.15%> (-0.49%) ⬇️
src/aggregation/bucket/range.rs 96.23% <100.00%> (ø)
src/aggregation/bucket/term_agg.rs 97.95% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@fulmicoton
Copy link
Collaborator

I am lacking background on this. What problem does it solve?

@PSeitz
Copy link
Contributor Author

PSeitz commented Apr 14, 2023

For sending intermediate results between nodes JSON is unnecessary brittle. It's easy to run into cases that are not supported without noticing.

Currently it fails sometimes for ddsketch deserialization, when there are f64 values with f64::INF, which serializes to null.
Same for f64::NAN serde-rs/json#202

I'd like to avoid any special de/serialization that can be handled by switching the format.
I tested serde round-trips on a similar data-structure here:

https://github.com/PSeitz/test_serde_formats

Format Result
Json Err(invalid type: null, expected f64 at line 1 column 81)
Postcard Err(Found an Option discriminant that wasn't 0 or 1)
Ron Ok(())
MessagePack Err(invalid length 0, expected struct IntermediateAggregationResults with 2 elements)
Bincode Err(invalid value: integer 10, expected variant index 0 <= i < 2)
Ciborium Ok(())
Bson Err(Invalid map key type: 10)

@fulmicoton
Copy link
Collaborator

Is the problem only NaN or is there something else?

@PSeitz
Copy link
Contributor Author

PSeitz commented Apr 15, 2023

Is the problem only NaN or is there something else?

For JSON it's NaN, INF,NEG_INF and non-string keys in Hashmaps. Maybe more we don't know yet

@PSeitz PSeitz force-pushed the agg_multiype branch 2 times, most recently from 953610f to e498909 Compare April 26, 2023 06:55
add support for other formats by removing skip_serialize and untagged
JSON support is broken anyway due it's lack on f64::INF etc. handling
src/aggregation/mod.rs Outdated Show resolved Hide resolved
@PSeitz PSeitz merged commit c599bf3 into main Apr 26, 2023
@PSeitz PSeitz deleted the agg_multiype branch April 26, 2023 11:05
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