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

compact: implement native histogram downsampling #8110

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

GiedriusS
Copy link
Member

@GiedriusS GiedriusS commented Feb 17, 2025

Update #6350:

  • Add some tests from me
  • Update to the newest API changes
  • Refactored downsampling loop to be clearer like I've commented
  • Updated Sebastian's tests to have proper "sum"
  • Unify "sum" in the histogram aggrchunks to be like in the float aggrchunks
  • Added a blurb to compactor's documentation
  • Added sample e2e test

Closes #5907.
Closes #6350.

@GiedriusS GiedriusS force-pushed the implement_nativehistogram_downsampling branch 2 times, most recently from c39432a to 73ed78c Compare February 17, 2025 16:03
Signed-off-by: Giedrius Statkevičius <[email protected]>
@GiedriusS GiedriusS force-pushed the implement_nativehistogram_downsampling branch from 73ed78c to e4e7fa2 Compare February 17, 2025 16:11
@GiedriusS GiedriusS marked this pull request as ready for review February 18, 2025 11:48
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks. This is epic. Code LGTM. I have 2 questions.

  1. Since we don't aggregate on min and max, does it work with the existing iterator we use in Querier?
  2. Do we need to update ApplyCounterResetIterator to support native histograms?

return it.Err()
}

// expandHistogramChunkIterator reads all histograms from the iterator and appends them to buf.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment needs update. expandFloatHistogramChunkIterator

for _, c := range chks {
if prevEnc != c.Chunk.Encoding() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If previous encoding is float histogram and current encoding is normal histogram, can we downsample those chunks together since the input is same, float histogram?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Native Histogram Support
2 participants