-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-53917][CONNECT] Support large local relations - follow-ups #52973
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
| "materialize all chunks in memory.") | ||
| .version("4.1.0") | ||
| .longConf | ||
| .checkValue(_ > 0, "The batch size in bytes must be positive") |
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.
We should check if this value is greater than the chunk size value value as an initial step
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.
In the case of conflicts, this conf should be respected and the operation should error out as we wouldn't want to bypass an explicitly set max materialisation size (to avoid system failures)
| @@ -6003,6 +6003,19 @@ object SQLConf { | |||
| .bytesConf(ByteUnit.BYTE) | |||
| .createWithDefaultString("3GB") | |||
|
|
|||
| val LOCAL_RELATION_BATCH_OF_CHUNKS_SIZE_BYTES = | |||
| buildConf(SqlApiConfHelper.LOCAL_RELATION_BATCH_OF_CHUNKS_SIZE_BYTES_KEY) | |||
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.
We could update the name here to be a bit more explicit in the sense that this pertains to the maximum number of bytes that we will materialise in memory for the specific local relation
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.
(specific because multi-threading can result in multiple artifacts being materialised at once)
3ceed3c to
84a54fe
Compare
|
merging to master/4.1 |
### What changes were proposed in this pull request? This PR adds several fixes and improvements over #52613 which added support for 2GB+ local relations in Spark Connect. #### Uploading batches of chunks Currently, before caching the local relation on the server via `ChunkedCachedLocalRelation`, the client materializes all chunks in memory (1 schema chunk and N data chunks). This can lead to high memory pressure on the client when uploading very large local relations. In this PR, I'm changing how the client uploads the local relation. Instead of materializing all chunks in memory, the client will materialize a batch of chunks in memory, upload the batch of chunks to the server, and proceed to collecting the next batch of chunks. The size of the batch of chunks is controlled via `spark.sql.session.localRelationBatchOfChunksSizeBytes` (1GB by default). This way, the uploading mechanism only consumes 1GB of memory at each point in time. Alternatives to this approach are: a) uploading each chunk separately - would require one pair of `ArtifactStatuses` and `AddArtifactsRequest` RPC calls for each chunk, which is inefficient. b) (current implementation) materializing all chunks in memory and uploading them via a single pair of `ArtifactStatuses` and `AddArtifactsRequest` RPC calls. This can lead to high memory pressure on the client. Uploading batches of chunks is a middle-ground solution. Changes are implemented both for the python and scala clients. #### Minor fixes and improvements - Replace `ArraySeq.unsafeWrapArray(data.map(_.copy()).toArray))` with `data.map(_.copy()).toArray.toImmutableArraySeq` in `SparkConnectPlanner.scala`. The latter is compatible with both scala 2.13 and scala 2.12 and is consistent with how arrays are converted to sequences in other places in the code base. - Improved asserts and tests in the python client. ### Why are the changes needed? Reduce memory pressure in the spark connect python and scala clients when uploading very large local relations to the server. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes #52973 from khakhlyuk/largelocalrelations-followup. Authored-by: Alex Khakhlyuk <[email protected]> Signed-off-by: Herman van Hovell <[email protected]> (cherry picked from commit 40ba971) Signed-off-by: Herman van Hovell <[email protected]>
|
Thank you, @khakhlyuk and @hvanhovell . |
|
@khakhlyuk, I hit |
|
@khakhlyuk, yes, I use an older version 4.1.0-preview3 connect server for testing. do we have a compatible policy for connect client/server, if it requires client version must be <= server version, I think we can ignore the issue. |
|
@pan3793 yes, exactly, the policy is to maintain forward compatibility - new servers have to support old clients. So client version <= server version. |
### What changes were proposed in this pull request? This PR adds several fixes and improvements over apache#52613 which added support for 2GB+ local relations in Spark Connect. #### Uploading batches of chunks Currently, before caching the local relation on the server via `ChunkedCachedLocalRelation`, the client materializes all chunks in memory (1 schema chunk and N data chunks). This can lead to high memory pressure on the client when uploading very large local relations. In this PR, I'm changing how the client uploads the local relation. Instead of materializing all chunks in memory, the client will materialize a batch of chunks in memory, upload the batch of chunks to the server, and proceed to collecting the next batch of chunks. The size of the batch of chunks is controlled via `spark.sql.session.localRelationBatchOfChunksSizeBytes` (1GB by default). This way, the uploading mechanism only consumes 1GB of memory at each point in time. Alternatives to this approach are: a) uploading each chunk separately - would require one pair of `ArtifactStatuses` and `AddArtifactsRequest` RPC calls for each chunk, which is inefficient. b) (current implementation) materializing all chunks in memory and uploading them via a single pair of `ArtifactStatuses` and `AddArtifactsRequest` RPC calls. This can lead to high memory pressure on the client. Uploading batches of chunks is a middle-ground solution. Changes are implemented both for the python and scala clients. #### Minor fixes and improvements - Replace `ArraySeq.unsafeWrapArray(data.map(_.copy()).toArray))` with `data.map(_.copy()).toArray.toImmutableArraySeq` in `SparkConnectPlanner.scala`. The latter is compatible with both scala 2.13 and scala 2.12 and is consistent with how arrays are converted to sequences in other places in the code base. - Improved asserts and tests in the python client. ### Why are the changes needed? Reduce memory pressure in the spark connect python and scala clients when uploading very large local relations to the server. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#52973 from khakhlyuk/largelocalrelations-followup. Authored-by: Alex Khakhlyuk <[email protected]> Signed-off-by: Herman van Hovell <[email protected]>
### What changes were proposed in this pull request? This PR adds several fixes and improvements over apache#52613 which added support for 2GB+ local relations in Spark Connect. #### Uploading batches of chunks Currently, before caching the local relation on the server via `ChunkedCachedLocalRelation`, the client materializes all chunks in memory (1 schema chunk and N data chunks). This can lead to high memory pressure on the client when uploading very large local relations. In this PR, I'm changing how the client uploads the local relation. Instead of materializing all chunks in memory, the client will materialize a batch of chunks in memory, upload the batch of chunks to the server, and proceed to collecting the next batch of chunks. The size of the batch of chunks is controlled via `spark.sql.session.localRelationBatchOfChunksSizeBytes` (1GB by default). This way, the uploading mechanism only consumes 1GB of memory at each point in time. Alternatives to this approach are: a) uploading each chunk separately - would require one pair of `ArtifactStatuses` and `AddArtifactsRequest` RPC calls for each chunk, which is inefficient. b) (current implementation) materializing all chunks in memory and uploading them via a single pair of `ArtifactStatuses` and `AddArtifactsRequest` RPC calls. This can lead to high memory pressure on the client. Uploading batches of chunks is a middle-ground solution. Changes are implemented both for the python and scala clients. #### Minor fixes and improvements - Replace `ArraySeq.unsafeWrapArray(data.map(_.copy()).toArray))` with `data.map(_.copy()).toArray.toImmutableArraySeq` in `SparkConnectPlanner.scala`. The latter is compatible with both scala 2.13 and scala 2.12 and is consistent with how arrays are converted to sequences in other places in the code base. - Improved asserts and tests in the python client. ### Why are the changes needed? Reduce memory pressure in the spark connect python and scala clients when uploading very large local relations to the server. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#52973 from khakhlyuk/largelocalrelations-followup. Authored-by: Alex Khakhlyuk <[email protected]> Signed-off-by: Herman van Hovell <[email protected]>
What changes were proposed in this pull request?
This PR adds several fixes and improvements over #52613 which added support for 2GB+ local relations in Spark Connect.
Uploading batches of chunks
Currently, before caching the local relation on the server via
ChunkedCachedLocalRelation, the client materializes all chunks in memory (1 schema chunk and N data chunks). This can lead to high memory pressure on the client when uploading very large local relations.In this PR, I'm changing how the client uploads the local relation. Instead of materializing all chunks in memory, the client will materialize a batch of chunks in memory, upload the batch of chunks to the server, and proceed to collecting the next batch of chunks. The size of the batch of chunks is controlled via
spark.sql.session.localRelationBatchOfChunksSizeBytes(1GB by default). This way, the uploading mechanism only consumes 1GB of memory at each point in time. Alternatives to this approach are:a) uploading each chunk separately - would require one pair of
ArtifactStatusesandAddArtifactsRequestRPC calls for each chunk, which is inefficient.b) (current implementation) materializing all chunks in memory and uploading them via a single pair of
ArtifactStatusesandAddArtifactsRequestRPC calls. This can lead to high memory pressure on the client.Uploading batches of chunks is a middle-ground solution.
Changes are implemented both for the python and scala clients.
Minor fixes and improvements
ArraySeq.unsafeWrapArray(data.map(_.copy()).toArray))withdata.map(_.copy()).toArray.toImmutableArraySeqinSparkConnectPlanner.scala. The latter is compatible with both scala 2.13 and scala 2.12 and is consistent with how arrays are converted to sequences in other places in the code base.Why are the changes needed?
Reduce memory pressure in the spark connect python and scala clients when uploading very large local relations to the server.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing tests.
Was this patch authored or co-authored using generative AI tooling?
No