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.

Sign up for GitHub

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

Jump to bottom

Spillable host buffer #9070

Merged
merged 5 commits into from
Aug 21, 2023
Merged

Spillable host buffer #9070

merged 5 commits into from
Aug 21, 2023

Conversation

abellina
Copy link
Collaborator

This contributes towards #8882 but it doesn't close it since we need to handle HostColumnVectors (it will end up being a SpillableHostBatch for now).

I can add more tests around host spillability but I do have a couple of TODOs in the code that I need some clarification on (possibly just put it in as is and add follow on as we add more support for host spillable).

Posting for some 👀 to make sure I didn't miss something major.

@abellina
Spillable host buffer
8a1c111 
Signed-off-by: Alessandro Bellina <[email protected]>
@abellina
Copy link
Collaborator Author

Markdown failing on:

2023-08-17T19:33:20.6747888Z ERROR: 2 dead links found!
2023-08-17T19:33:20.6748263Z [✖] https://repo1.maven.org/maven2/com/nvidia/rapids-4-spark_2.12/23.08.1/rapids-4-spark_2.12-23.08.1.jar → Status: 404
2023-08-17T19:33:20.6748643Z [✖] https://repo1.maven.org/maven2/com/nvidia/rapids-4-spark_2.12/23.08.1/rapids-4-spark_2.12-23.08.1.jar.asc → Status: 404

revans2
revans2 previously approved these changes Aug 17, 2023
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Looks good. Need to start using it to really see what we need to change, but generally it looks a lot smaller than I expected.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsBufferCatalog.scala Outdated
@@ -294,29 +295,42 @@ class RapidsBufferCatalog(
}

/**
* Adds a buffer to the device storage. This does NOT take ownership of the
* buffer, so it is the responsibility of the caller to close it.
* Adds a buffer to the either device or host storage. This does NOT take
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit Adds a buffer to either the device or host storage.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsBufferCatalog.scala Show resolved Hide resolved
@sameerz sameerz added the reliability Features to improve reliability or bugs that severly impact the reliability of the plugin label Aug 17, 2023
@abellina abellina marked this pull request as ready for review August 18, 2023 02:54
@abellina
Copy link
Collaborator Author

build

revans2
revans2 previously approved these changes Aug 18, 2023
View reviewed changes
@revans2
Copy link
Collaborator

revans2 commented Aug 18, 2023

@jlowe you looked at the first spillable code changes for device buffers. Could you take a look at this one too, just to have another pair of eyes so to find things I might have missed?

@abellina
Copy link
Collaborator Author

Ok sorry for the noise. Logging was wrong with my patch, and I've fixed it now. @revans2 @jlowe

@abellina abellina requested review from jlowe and revans2 August 18, 2023 15:17
jlowe
jlowe approved these changes Aug 18, 2023
View reviewed changes
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Got confused by the setSpillable/doSetSpillable distinction, but that already existed and can be addressed separately, tracked in #9076.

@abellina
Copy link
Collaborator Author

build

@abellina abellina merged commit f723dfc into NVIDIA:branch-23.10 Aug 21, 2023
26 of 27 checks passed
@abellina abellina deleted the spillable_host_buffer branch August 21, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlowe jlowe jlowe approved these changes

@revans2 revans2 Awaiting requested review from revans2

Assignees
No one assigned
Labels
reliability Features to improve reliability or bugs that severly impact the reliability of the plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abellina @revans2 @jlowe @sameerz