Skip to content

[C++] Remove capacity() check in TDigest::TDigestImpl::Validate() - #52276

Draft
ezbr wants to merge 3 commits into
apache:mainfrom
ezbr:tdigest-validate-size
Draft

ezbr wants to merge 3 commits into
apache:mainfrom
ezbr:tdigest-validate-size

Conversation

@ezbr

@ezbr ezbr commented Oct 7, 2026 •

Copy link
Copy Markdown

Rationale for this change

std::vector::reserve(delta_) only guarantees capacity() >= delta_, not capacity() == delta_. When the C++ standard library and allocator use size-returning allocations (such as C++23 allocate_at_least), reserve(delta_) can immediately set capacity() > delta_, causing TDigestImpl::Validate() to fail with "oversized tdigest buffer" even when the number of centroids is at most delta_.

Fixes issue #53777.

What changes are included in this PR?

Remove the check of capacity() > delta_ in TDigest::TDigestImpl::Validate().

Are these changes tested?

Yes, covered by existing TDigestTest unit tests in cpp/src/arrow/util/tdigest_test.cc.

Are there any user-facing changes?

No.

Was AI used for this PR?

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

…pl::Validate()

`std::vector::reserve(delta_)` only guarantees `capacity() >= delta_`, not `capacity() == delta_`. When the standard library/allocator uses size-returning allocations (such as C++23 `allocate_at_least`), `reserve(delta_)` can immediately set `capacity() > delta_`, causing `TDigestImpl::Validate()` to fail with "oversized tdigest buffer" even when the number of centroids is at most `delta_`.

Check `size() > delta_` instead of `capacity() > delta_` to verify that the tdigest buffer did not expand past `delta_` elements.
Comment thread cpp/src/arrow/util/tdigest.cc Outdated
@ezbr

ezbr commented Oct 12, 2026

Copy link
Copy Markdown
Author

Hi Dmitry, can you please merge this PR? Do we need additional reviewers?

Thanks,
Evan

@dmitry-chirkov-dremio

Copy link
Copy Markdown
Contributor

Hi Dmitry, can you please merge this PR? Do we need additional reviewers?
I am not a maintainer so can't merge.

See the guide here.

@ezbr

ezbr commented Oct 13, 2026

Copy link
Copy Markdown
Author

@pitrou Can you please take a look and/or suggest a different maintainer as a reviewer?

Thanks,
Evan

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 14, 2026
@pitrou

pitrou commented Oct 14, 2026

Copy link
Copy Markdown
Member

@ezbr This does not look like a MINOR issue as it's fixing a potential bug, can you open an issue for it?

@cyb70289 Would you like to take a look at this PR? You are the main author of the TDigest code.

Comment thread cpp/src/arrow/util/tdigest.cc Outdated
@ezbr ezbr changed the title MINOR: [C++] Check size() instead of capacity() in TDigest::TDigestImpl::Validate() [C++] Remove capacity() check in TDigest::TDigestImpl::Validate() Oct 14, 2026
@ezbr

ezbr commented Oct 14, 2026

Copy link
Copy Markdown
Author

This does not look like a MINOR issue as it's fixing a potential bug, can you open an issue for it?

I filed issue #53777.

@ezbr
ezbr requested a review from cyb70289 October 14, 2026 18:17
@github-actions
github-actions Bot marked this pull request as draft October 14, 2026 18:17
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.

4 participants