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

fix: Fix the memory reclaim bytes for hash join #11642

Closed
wants to merge 1 commit into from

Conversation

xiaoxmeng
Copy link
Contributor

Summary:
Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Differential Revision: D66437719

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 25, 2024
Copy link

netlify bot commented Nov 25, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit fbfb1c1
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6747be22a512520008d48093

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66437719

Copy link
Contributor

@tanjialiang tanjialiang left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement. Should we have a test for checking node level reclaimed memory?

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Nov 26, 2024
)

Summary:

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Differential Revision: D66437719
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66437719

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Nov 26, 2024
)

Summary:

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Differential Revision: D66437719
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66437719

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Nov 26, 2024
)

Summary:

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Differential Revision: D66437719
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66437719

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Nov 26, 2024
)

Summary:

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Differential Revision: D66437719
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66437719

@xiaoxmeng xiaoxmeng changed the title fix: fix the memory reclaim bytes for hash join fix: Fix the memory reclaim bytes for hash join Nov 26, 2024
xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Nov 27, 2024
)

Summary:

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Reviewed By: bikramSingh91, tanjialiang

Differential Revision: D66437719
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66437719

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Nov 27, 2024
)

Summary:

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Reviewed By: bikramSingh91, tanjialiang

Differential Revision: D66437719
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66437719

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Nov 27, 2024
)

Summary:

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Reviewed By: bikramSingh91, tanjialiang

Differential Revision: D66437719
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66437719

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Nov 27, 2024
)

Summary:

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Reviewed By: bikramSingh91, tanjialiang

Differential Revision: D66437719
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66437719

)

Summary:

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Reviewed By: bikramSingh91, tanjialiang

Differential Revision: D66437719
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66437719

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 1ce3c7a.

@xiaoxmeng xiaoxmeng deleted the export-D66437719 branch November 28, 2024 06:12
TongWei1105 pushed a commit to TongWei1105/velox that referenced this pull request Dec 3, 2024
)

Summary:
Pull Request resolved: facebookincubator#11642

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Reviewed By: bikramSingh91, tanjialiang

Differential Revision: D66437719

fbshipit-source-id: 4ac7bb9cf87b4346d234ef5f7c04ed64ee12d249
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
)

Summary:
Pull Request resolved: facebookincubator#11642

Both hash join and probe does the coordinated spill so we shouldn't report the reclaimed bytes from a single node
but shall report from the plan node. Also probe side spill might spill built table from join side and the memory is
actually reclaimed from build side pool instead of probe side.

This PR also removes the unused wait for spill state from hash build

Reviewed By: bikramSingh91, tanjialiang

Differential Revision: D66437719

fbshipit-source-id: 4ac7bb9cf87b4346d234ef5f7c04ed64ee12d249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants