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

Consolidate SpillStats #9211

Closed
wants to merge 1 commit into from
Closed

Conversation

duanmeng
Copy link
Collaborator

@duanmeng duanmeng commented Mar 22, 2024

Decouple spill stats from the spiller as row number and hash probe spilling might
use more than one and different spillers. Consolidate to use one spill stats to collect
the spill stats to streamline implementation. This PR introduces a synchronized spill
stats within the operator to gather these stats and later on we could separate them
for different types of spiller if offline analysis needs.

@duanmeng duanmeng requested a review from xiaoxmeng March 22, 2024 07:01
@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 Mar 22, 2024
@duanmeng duanmeng marked this pull request as draft March 22, 2024 07:01
Copy link

netlify bot commented Mar 22, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 016b493
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65ff86fc250ce60008e411ba

Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@duanmeng the approach looks good to me. Thanks!

velox/exec/Spiller.h Outdated Show resolved Hide resolved
velox/exec/SortBuffer.h Outdated Show resolved Hide resolved
velox/exec/SortWindowBuild.h Outdated Show resolved Hide resolved
@duanmeng duanmeng force-pushed the refactor branch 8 times, most recently from 6ba9973 to 7a997a9 Compare March 22, 2024 17:44
@duanmeng duanmeng marked this pull request as ready for review March 22, 2024 17:45
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@duanmeng thanks!

velox/connectors/hive/HiveDataSink.h Show resolved Hide resolved
velox/exec/Operator.cpp Outdated Show resolved Hide resolved
velox/exec/HashProbe.cpp Outdated Show resolved Hide resolved
velox/exec/Operator.cpp Show resolved Hide resolved
@duanmeng duanmeng changed the title Refactor SpillStats Consolidate SpillStats Mar 23, 2024
@duanmeng duanmeng force-pushed the refactor branch 4 times, most recently from 3010211 to e8e004b Compare March 23, 2024 13:40
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@duanmeng LGTM. Thanks!

velox/exec/TableWriter.cpp Outdated Show resolved Hide resolved
velox/exec/HashBuild.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@duanmeng thanks for the update!

@facebook-github-bot
Copy link
Contributor

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@xiaoxmeng merged this pull request in 8f0adb1.

Copy link

Conbench analyzed the 1 benchmark run on commit 8f0adb1f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

wypb pushed a commit to wypb/velox that referenced this pull request Mar 25, 2024
Summary:
Decouple spill stats from the spiller as row number and hash probe spilling might
use more than one and different spillers. Consolidate to use one spill stats to collect
the spill stats to streamline implementation. This PR introduces a synchronized spill
stats within the operator to gather these stats and later on we could separate them
for different types of spiller if offline analysis needs.

Pull Request resolved: facebookincubator#9211

Reviewed By: tanjialiang

Differential Revision: D55287100

Pulled By: xiaoxmeng

fbshipit-source-id: ffde57d4f3425e3f3f679252504f7690e8dfce68
wypb pushed a commit to wypb/velox that referenced this pull request Mar 25, 2024
Summary:
Decouple spill stats from the spiller as row number and hash probe spilling might
use more than one and different spillers. Consolidate to use one spill stats to collect
the spill stats to streamline implementation. This PR introduces a synchronized spill
stats within the operator to gather these stats and later on we could separate them
for different types of spiller if offline analysis needs.

Pull Request resolved: facebookincubator#9211

Reviewed By: tanjialiang

Differential Revision: D55287100

Pulled By: xiaoxmeng

fbshipit-source-id: ffde57d4f3425e3f3f679252504f7690e8dfce68
wypb pushed a commit to wypb/velox that referenced this pull request Mar 25, 2024
Summary:
Decouple spill stats from the spiller as row number and hash probe spilling might
use more than one and different spillers. Consolidate to use one spill stats to collect
the spill stats to streamline implementation. This PR introduces a synchronized spill
stats within the operator to gather these stats and later on we could separate them
for different types of spiller if offline analysis needs.

Pull Request resolved: facebookincubator#9211

Reviewed By: tanjialiang

Differential Revision: D55287100

Pulled By: xiaoxmeng

fbshipit-source-id: ffde57d4f3425e3f3f679252504f7690e8dfce68
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
Summary:
Decouple spill stats from the spiller as row number and hash probe spilling might
use more than one and different spillers. Consolidate to use one spill stats to collect
the spill stats to streamline implementation. This PR introduces a synchronized spill
stats within the operator to gather these stats and later on we could separate them
for different types of spiller if offline analysis needs.

Pull Request resolved: facebookincubator#9211

Reviewed By: tanjialiang

Differential Revision: D55287100

Pulled By: xiaoxmeng

fbshipit-source-id: ffde57d4f3425e3f3f679252504f7690e8dfce68
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. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants