Skip to content

fix: HashTable dynamic rehash and group count limit for GROUP BY aggregation#48174

Open
MrPresent-Han wants to merge 1 commit intomilvus-io:masterfrom
MrPresent-Han:hashtable-rehash-master
Open

fix: HashTable dynamic rehash and group count limit for GROUP BY aggregation#48174
MrPresent-Han wants to merge 1 commit intomilvus-io:masterfrom
MrPresent-Han:hashtable-rehash-master

Conversation

@MrPresent-Han
Copy link
Contributor

Summary

  • HashTable now dynamically rehashes (doubles capacity) when load factor exceeds 7/8, fixing crash when GROUP BY cardinality > ~1792
  • Added configurable queryNode.segcore.maxGroupByGroups (default 100K) to cap total groups and prevent OOM on both C++ (per-segment HashTable) and Go (cross-segment agg reducer) layers
  • Added 4 C++ unit tests covering rehash basic/correctness, max groups limit, and multiple rehash rounds

issue: #47569

Test plan

  • C++ unit tests: --gtest_filter="*HashTableRehash*:*MaxGroups*"
  • E2E: GROUP BY aggregation with >2K unique values should succeed
  • E2E: Set queryNode.segcore.maxGroupByGroups to small value, verify clear error message

🤖 Generated with Claude Code

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MrPresent-Han
To complete the pull request process, please assign tedxu after the PR has been reviewed.
You can assign the PR to them by writing /assign @tedxu in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Mar 10, 2026
@mergify mergify bot added dco-passed DCO check passed. kind/bug Issues or changes related a bug labels Mar 10, 2026
@sre-ci-robot
Copy link
Contributor

[ci-v2-notice]
Notice: New ci-v2 system is enabled for this PR.

To rerun ci-v2 checks, comment with:

  • /ci-rerun-code-check // for ci-v2/code-check
  • /ci-rerun-build // for ci-v2/build
  • /ci-rerun-build-all // for ci-v2/build-all (multi-arch builds)
  • /ci-rerun-ut-integration // for ci-v2/ut-integration, will rerun ci-v2/build
  • /ci-rerun-ut-go // for ci-v2/ut-go, will rerun ci-v2/build
  • /ci-rerun-ut-cpp // for ci-v2/ut-cpp
  • /ci-rerun-ut // for all ci-v2/ut-integration, ci-v2/ut-go, ci-v2/ut-cpp, will rerun ci-v2/build
  • /ci-rerun-e2e-arm // for ci-v2/e2e-arm
  • /ci-rerun-e2e-default // for ci-v2/e2e-default
  • /ci-rerun-ciloop // for ci-v2/ciloop (build + unit tests in one pipeline)

If you have any questions or requests, please contact @zhikunyao.

@mergify
Copy link
Contributor

mergify bot commented Mar 10, 2026

@MrPresent-Han go-sdk check failed, comment rerun go-sdk can trigger the job again.

@codecov
Copy link

codecov bot commented Mar 10, 2026

Codecov Report

❌ Patch coverage is 92.20779% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.27%. Comparing base (77829de) to head (a09924e).
⚠️ Report is 51 commits behind head on master.

Files with missing lines Patch % Lines
internal/core/src/segcore/segcore_init_c.cpp 0.00% 4 Missing ⚠️
internal/agg/aggregate_reducer.go 75.00% 1 Missing and 2 partials ⚠️
pkg/util/paramtable/component_param.go 76.92% 2 Missing and 1 partial ⚠️
internal/core/src/exec/HashTable.cpp 96.42% 1 Missing ⚠️
internal/core/src/segcore/SegcoreConfig.h 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #48174      +/-   ##
==========================================
- Coverage   77.29%   77.27%   -0.03%     
==========================================
  Files        2080     2080              
  Lines      342401   342544     +143     
==========================================
+ Hits       264654   264690      +36     
- Misses      69492    69575      +83     
- Partials     8255     8279      +24     
Components Coverage Δ
Client 78.62% <ø> (ø)
Core 83.82% <95.23%> (+0.02%) ⬆️
Go 75.47% <78.57%> (-0.05%) ⬇️
Files with missing lines Coverage Δ
internal/core/src/exec/HashTable.h 98.14% <100.00%> (+7.58%) ⬆️
...l/core/src/exec/operator/query-agg/GroupingSet.cpp 100.00% <100.00%> (ø)
internal/core/unittest/test_query_group_by.cpp 99.88% <100.00%> (+0.01%) ⬆️
internal/util/initcore/query_node.go 79.23% <100.00%> (+0.49%) ⬆️
internal/core/src/exec/HashTable.cpp 97.35% <96.42%> (+3.85%) ⬆️
internal/core/src/segcore/SegcoreConfig.h 80.95% <50.00%> (-1.55%) ⬇️
internal/agg/aggregate_reducer.go 49.16% <75.00%> (+1.70%) ⬆️
pkg/util/paramtable/component_param.go 97.09% <76.92%> (-0.05%) ⬇️
internal/core/src/segcore/segcore_init_c.cpp 17.59% <0.00%> (-0.68%) ⬇️

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sre-ci-robot sre-ci-robot added the low-code-coverage add test-label from zhikun, diff coverage > 80% label Mar 10, 2026
@MrPresent-Han MrPresent-Han force-pushed the hashtable-rehash-master branch 2 times, most recently from 4bb6ab8 to c1e903b Compare March 11, 2026 04:05
@sre-ci-robot sre-ci-robot removed the low-code-coverage add test-label from zhikun, diff coverage > 80% label Mar 11, 2026
@mergify mergify bot added the ci-passed label Mar 11, 2026
@MrPresent-Han MrPresent-Han force-pushed the hashtable-rehash-master branch from c1e903b to d47f238 Compare March 12, 2026 02:21
@sre-ci-robot sre-ci-robot added low-code-coverage add test-label from zhikun, diff coverage > 80% and removed ci-passed low-code-coverage add test-label from zhikun, diff coverage > 80% labels Mar 12, 2026
@mergify mergify bot added the ci-passed label Mar 12, 2026
@MrPresent-Han MrPresent-Han force-pushed the hashtable-rehash-master branch from d47f238 to 8330542 Compare March 12, 2026 12:28
@MrPresent-Han
Copy link
Contributor Author

has verified by cc reviewers team

@sre-ci-robot sre-ci-robot added low-code-coverage add test-label from zhikun, diff coverage > 80% and removed ci-passed low-code-coverage add test-label from zhikun, diff coverage > 80% labels Mar 12, 2026
@mergify mergify bot added the ci-passed label Mar 12, 2026
@MrPresent-Han MrPresent-Han force-pushed the hashtable-rehash-master branch from 8330542 to d75ad20 Compare March 16, 2026 12:51
@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2026

@MrPresent-Han go-sdk check failed, comment rerun go-sdk can trigger the job again.

@MrPresent-Han MrPresent-Han force-pushed the hashtable-rehash-master branch from d75ad20 to 010ea9a Compare March 17, 2026 02:21
@sre-ci-robot sre-ci-robot added the low-code-coverage add test-label from zhikun, diff coverage > 80% label Mar 17, 2026
@sre-ci-robot sre-ci-robot removed the low-code-coverage add test-label from zhikun, diff coverage > 80% label Mar 17, 2026
@MrPresent-Han MrPresent-Han force-pushed the hashtable-rehash-master branch from 010ea9a to 3511def Compare March 18, 2026 09:00
@sre-ci-robot sre-ci-robot added low-code-coverage add test-label from zhikun, diff coverage > 80% and removed low-code-coverage add test-label from zhikun, diff coverage > 80% labels Mar 18, 2026
…egation (milvus-io#47569)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
@MrPresent-Han MrPresent-Han force-pushed the hashtable-rehash-master branch from 3511def to a09924e Compare March 19, 2026 03:39
@mergify
Copy link
Contributor

mergify bot commented Mar 19, 2026

@MrPresent-Han go-sdk check failed, comment rerun go-sdk can trigger the job again.

@sre-ci-robot sre-ci-robot added the low-code-coverage add test-label from zhikun, diff coverage > 80% label Mar 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-passed DCO check passed. kind/bug Issues or changes related a bug low-code-coverage add test-label from zhikun, diff coverage > 80% size/L Denotes a PR that changes 100-499 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants