Skip to content

Add spill support in StreamingWindow addInput() Phase#8631

Closed
JkSelf wants to merge 1 commit into
facebookincubator:mainfrom
JkSelf:streamingwindow-spill
Closed

Add spill support in StreamingWindow addInput() Phase#8631
JkSelf wants to merge 1 commit into
facebookincubator:mainfrom
JkSelf:streamingwindow-spill

Conversation

@JkSelf
Copy link
Copy Markdown
Collaborator

@JkSelf JkSelf commented Feb 1, 2024

@netlify
Copy link
Copy Markdown

netlify Bot commented Feb 1, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 47b7bf6
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65e9694cfbe36a0008f59381

@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 Feb 1, 2024
@JkSelf JkSelf force-pushed the streamingwindow-spill branch from 5459dcc to 479eef3 Compare February 1, 2024 08:37
@JkSelf JkSelf marked this pull request as draft February 1, 2024 08:37
@JkSelf JkSelf force-pushed the streamingwindow-spill branch 2 times, most recently from 73c2569 to 94d5c70 Compare March 6, 2024 03:06
Comment thread velox/exec/StreamingWindowBuild.cpp Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It seems that the data of a partition is preset here to be fully stored in memory. But in the case of spill, the data that comes back from the spill file often cannot fit into memory. What if part of it is in memory and part of it is in the spill file.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@guhaiyan0221 Currently, only one window partition is supported, either it is in memory or it has been spilled. It does not support a part in memory and a part has been spilled. The reason why the data needs to be loaded back into memory here is because the WindowPartition only accepts data in memory and does not accept spilled data. This is also the second step to do. Thanks.

@JkSelf JkSelf force-pushed the streamingwindow-spill branch 4 times, most recently from 7ef7d26 to d7c682c Compare March 7, 2024 04:12
@JkSelf JkSelf marked this pull request as ready for review March 7, 2024 04:16
@JkSelf
Copy link
Copy Markdown
Collaborator Author

JkSelf commented Mar 7, 2024

@mbasmanova @aditi-pandit @zhztheplayer @guhaiyan0221 @zhouyuan Can you help to review? Thanks.

@JkSelf JkSelf force-pushed the streamingwindow-spill branch 2 times, most recently from 36af0ce to 47b7bf6 Compare March 7, 2024 07:14
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@JkSelf As discussed in #8975, I do not think it is a good idea to add spilling support to StreamingWindow.

@stale
Copy link
Copy Markdown

stale Bot commented Jun 6, 2024

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

@stale stale Bot added the stale label Jun 6, 2024
@stale stale Bot closed this Jun 20, 2024
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. stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants