-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Make spilling hash builder better report reserved memory #25989
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
Conversation
- reserve memory before creating a LookupSource - yield if unspilled pages cannot fit memory immediately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the HashBuilderOperator to better handle reserved memory during lookup source creation and unspilling, and updates revocable memory after compaction. It also adds new tests to validate these memory-waiting behaviors under spill/no-spill scenarios.
- Reserve memory before building the lookup source in
finishInputandfinishLookupSourceUnspilling, and yield if reservation is pending. - Update
startMemoryRevoketo adjust the revocable memory context after compaction. - Add
DriverTestContexthelper and new tests inTestHashJoinOperatorto verify that the operator waits for memory when finishing input and unspilling.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java | Added pre-reservation of memory contexts in finishInput, unspill logic changes, and compaction update in startMemoryRevoke. |
| core/trino-main/src/test/java/io/trino/operator/join/TestHashJoinOperator.java | Introduced DriverTestContext helper and new tests to assert that HashBuilderOperator waits for memory under spill/unspill conditions. |
Comments suppressed due to low confidence (2)
core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java:513
- [nitpick] Add a comment explaining that
finishInputshould be retried once the pending memory reservation completes, so readers understand why we return early here.
if (!reserved.isDone()) {
core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java:587
- [nitpick] The flag
unspilledPagesAddedcould be renamed to something likepagesUnspilledorunspillCompletedto more clearly reflect that it tracks whether pages have been transferred into the index.
if (!unspilledPagesAdded) {
core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/operator/join/TestHashJoinOperator.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java
Outdated
Show resolved
Hide resolved
3b7da8e to
dbb8e62
Compare
This reduces congestion in the memory tracking system by coarsening the granularity of memory tracking. In this paradigm, the most small incremental increases at the byte granularity will not actually result in a different coarse granularity value, so no reporting into memory tracking is necessary.
2c6594c to
bc1bc31
Compare
core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java
Show resolved
Hide resolved
raunaqmorarka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % test failures
|
flakes due to #21078 |
|
cc @osscm |
Description
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:
Fixes #25976