Skip to content

fix: stop REQ section growth at the minimum section size - #566

Open
proost wants to merge 5 commits into
apache:masterfrom
proost:fix-stop-section-growth
Open

proost wants to merge 5 commits into
apache:masterfrom
proost:fix-stop-section-growth

Conversation

@proost

@proost proost commented Nov 6, 2026

Copy link
Copy Markdown
Member

ensure_enough_sections() doubled the number of sections even when the
section size was already MIN_K and the reduced size rounded back to
MIN_K. The nominal capacity then grew 2x instead of the sqrt(2).

This is same as java.

@proost
proost requested a review from leerho November 6, 2026 08:12
@proost proost self-assigned this Nov 6, 2026

@leerho leerho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See comment in main thread.

@leerho

leerho commented Nov 6, 2026

Copy link
Copy Markdown
Member

Thanks, Hyeonho. The change looks right to me: it adds the same sectionSize > MIN_K stop that ReqCompactor.ensureEnoughSections() has in Java, and the new test fails without it (k = 14 gives a nominal capacity of 384 instead of 192).

One small request: please restore the commented-out "for manual comparison with Java" test, or move its removal to a separate change, since it isn't related to this fix.

We're finishing a bug-fix release and holding other PRs until it's out, so this will wait until then. It only affects a few small values of k, and the sketch is larger than designed rather than wrong, so it doesn't need to go into that release. It may need a small test adjustment at merge time, depending on what lands on master first.

@proost
proost requested a review from leerho November 7, 2026 06:31

This branch has not been deployed

No deployments
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.

2 participants